Skip to content

[FSSDK-9632] feat: Implement notification consistency across agent nodes #399

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
Merged
merged 36 commits into from
Nov 15, 2023
Merged

[FSSDK-9632] feat: Implement notification consistency across agent nodes #399

merged 36 commits into from
Nov 15, 2023

Conversation

pulak-opti
Copy link
Contributor

Summary

  • Implement notification consistency across agent nodes

Tickets

@pulak-opti pulak-opti mentioned this pull request Sep 13, 2023
Merged
@pulak-opti pulak-opti marked this pull request as ready for review September 15, 2023 13:11
mikechu-optimizely
mikechu-optimizely approved these changes Sep 15, 2023
View reviewed changes
Copy link
Contributor

@mikechu-optimizely mikechu-optimizely left a comment

Choose a reason for hiding this comment

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

Transparently: I do not completely understand the implementation details, but I trust your tests cover the functional additions. LGTM

jaeopt
jaeopt requested changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

Looks great!
A couple of changes suggested. I will review again after discussion.

@pulak-opti pulak-opti marked this pull request as draft October 23, 2023 10:55
@pulak-opti pulak-opti force-pushed the pulak/fix-k8s-notifications branch from a1b5849 to ce403f7 Compare October 23, 2023 17:44
@pulak-opti pulak-opti marked this pull request as ready for review October 25, 2023 13:20
@pulak-opti pulak-opti requested a review from jaeopt October 25, 2023 13:20
jaeopt
jaeopt requested changes Oct 28, 2023
View reviewed changes
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

Looks good with the sdkKey-based channels!
I want to consider a sync-er shared for all synchronizations supporting different types of events. We can chat about the options.

@pulak-opti pulak-opti requested a review from jaeopt November 6, 2023 11:30
jaeopt
jaeopt approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM - I want to discuss more about our options after we integrate datafile syncer.

@pulak-opti pulak-opti merged commit 9825f2d into master Nov 15, 2023
@pulak-opti pulak-opti deleted the pulak/fix-k8s-notifications branch November 15, 2023 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaeopt jaeopt jaeopt approved these changes

@mikechu-optimizely mikechu-optimizely mikechu-optimizely 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.
3 participants
@pulak-opti @jaeopt @mikechu-optimizely