Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(notification): allow multiple observer nodes on one worker #8210

Merged
merged 5 commits into from
Feb 28, 2023

Conversation

Gun9niR
Copy link
Contributor

@Gun9niR Gun9niR commented Feb 27, 2023

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Previously, if we want to send a notification by WorkerKey, it would be sent to all observer nodes of that worker, causing panic on unaccepted message types. This PR fixes it.

Another possible implementation would be using a single remote connection for each worker and dispatching notifications from ObserverManager on the worker node side, but that would affect the timeliness of notification handling.

Checklist For Contributors

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have demonstrated that backward compatibility is not broken by breaking changes and created issues to track deprecated features to be removed in the future. (Please refer to the issue)
  • All checks passed in ./risedev check (or alias, ./risedev c)

Checklist For Reviewers

Documentation

  • My PR DOES NOT contain user-facing changes.
Click here for Documentation

Types of user-facing changes

Please keep the types that apply to your changes, and remove the others.

  • Installation and deployment
  • Connector (sources & sinks)
  • SQL commands, functions, and operators
  • RisingWave cluster configuration changes
  • Other (please specify in the release note below)

Release note

@Gun9niR Gun9niR changed the title feat(notification): allow multiple observer nodes on one worker refactor(notification): allow multiple observer nodes on one worker Feb 27, 2023
@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Merging #8210 (6d07a58) into main (85646ba) will increase coverage by 0.00%.
The diff coverage is 89.87%.

@@           Coverage Diff           @@
##             main    #8210   +/-   ##
=======================================
  Coverage   71.58%   71.58%           
=======================================
  Files        1131     1131           
  Lines      182679   182720   +41     
=======================================
+ Hits       130775   130809   +34     
- Misses      51904    51911    +7     
Flag Coverage Δ
rust 71.58% <89.87%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/meta/src/rpc/service/notification_service.rs 0.00% <0.00%> (ø)
src/meta/src/manager/notification.rs 83.84% <90.90%> (+5.11%) ⬆️
...orage/hummock_test/src/mock_notification_client.rs 95.74% <100.00%> (ø)
src/meta/src/hummock/mock_hummock_meta_client.rs 64.43% <0.00%> (-1.04%) ⬇️
src/batch/src/task/task_execution.rs 51.62% <0.00%> (-0.76%) ⬇️
...frontend/src/scheduler/hummock_snapshot_manager.rs 60.00% <0.00%> (-0.48%) ⬇️
src/storage/src/hummock/sstable/multi_builder.rs 97.36% <0.00%> (-0.33%) ⬇️
src/storage/src/hummock/compactor/mod.rs 80.19% <0.00%> (-0.20%) ⬇️
src/stream/src/executor/aggregation/minput.rs 96.25% <0.00%> (-0.11%) ⬇️
src/meta/src/hummock/manager/mod.rs 75.62% <0.00%> (-0.07%) ⬇️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mergify
Copy link
Contributor

mergify bot commented Feb 27, 2023

Hey @Gun9niR, this pull request failed to merge and has been dequeued from the merge train. If you believe your PR failed in the merge train because of a flaky test, requeue it by clicking "Update branch" or pushing an empty commit with git commit --allow-empty -m "rerun" && git push.

@mergify
Copy link
Contributor

mergify bot commented Feb 27, 2023

Hey @Gun9niR, this pull request failed to merge and has been dequeued from the merge train. If you believe your PR failed in the merge train because of a flaky test, requeue it by clicking "Update branch" or pushing an empty commit with git commit --allow-empty -m "rerun" && git push.

@Gun9niR Gun9niR added this pull request to the merge queue Feb 28, 2023
Merged via the queue into main with commit c54070c Feb 28, 2023
@Gun9niR Gun9niR deleted the zhidong/notification branch February 28, 2023 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants