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.

Sign up for GitHub

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

Jump to bottom

add kafka announcement only logic #1027

Merged
merged 12 commits into from
Oct 15, 2024
Merged

add kafka announcement only logic #1027

merged 12 commits into from
Oct 15, 2024

Conversation

brycezhongqing
Copy link
Collaborator

Summary

add kafka announcement only logic

More context and test could be found here:
https://github.com/linkedin-multiproduct/container/pull/1162

bohhyang
bohhyang requested changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@bohhyang bohhyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my other comment in the container PR. ZookeeperAnnouncer needs to hold a general object, not a concrete ZookeeperServer

CHANGELOG.md Outdated Show resolved Hide resolved
d2/src/main/java/com/linkedin/d2/balancer/servers/ConnectionManager.java Show resolved Hide resolved
@brycezhongqing
Copy link
Collaborator Author

See my other comment in the container PR. ZookeeperAnnouncer needs to hold a general object, not a concrete ZookeeperServer
Updated. We are going to use LoadBalancerServer there

bohhyang
bohhyang reviewed Oct 11, 2024
View reviewed changes
gradle.properties Outdated
@@ -1,4 +1,4 @@
version=29.58.11
version=29.58.12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better bump the minor version since we changed the public API of ZooKeeperAnnouncer (although it's backward compatible).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@bohhyang bohhyang Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my other comment about emitting SD event in container PR. It will need some changes in the callbacks of various markup/down in ZookeeperAnnouncer.

Copy link
Collaborator Author

@brycezhongqing brycezhongqing Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will need some changes in the callbacks of various markup/down in ZookeeperAnnouncer.

How about directly change emitSDStatusActiveUpdateIntentAndWriteEvents ? We don't need to care about where it called

@brycezhongqing brycezhongqing merged commit bd54a98 into master Oct 15, 2024
2 checks passed
@brycezhongqing brycezhongqing deleted the zhonchen/announce_to_indis_only branch October 15, 2024 21:40
@zackthehuman zackthehuman mentioned this pull request Oct 17, 2024
Merged
dg-builder added a commit to dg-builder/rest.li that referenced this pull request Oct 18, 2024
@dg-builder
Revert "add kafka announcement only logic (linkedin#1027)"
83e1038 
This reverts commit bd54a98.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bohhyang bohhyang bohhyang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@brycezhongqing @bohhyang