Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Enhance CheckpointState to support no-op replication #5282
Enhance CheckpointState to support no-op replication #5282
Changes from 23 commits
46e1fc5
d8bdb51
619857b
76bfac3
53add62
ca9a3fa
1c71cae
0b812f8
fb96c02
d849b0e
6523c4d
71f0519
8318e78
043bfa1
71ade98
62d5959
e76ff6f
ad831e3
73113e1
c6d8e18
b252dbc
0f29d75
f3f63e8
897ef1c
d30afb3
16dc3fb
af59752
4aa2790
6e107f6
5a6fea1
366bfb0
3b22b38
2081f00
7b0ce5c
3c3fb27
bfbb71f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we send to the same
replicaRequest
in case ofLOGICAL_REPLICATION
andPRIMARY_TERM_VALIDATION
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for full replication and primary term validation both, we send the request to the replica shard. There will be a follow up PR that would bring a thin layer for primary term validation (basically perform primary term valdiation upfront and fail fast if stale).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the default constructor defined explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this abstraction of
ReplicationOverridePolicy
overReplicationMode
required?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for extensibility. Currently we have only the replication mode as a field, but this can expand for future usecases. For the sake of this PR, consider ReplicationOverridePolicy synonymous to ReplicationMode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use
ReplicationMode
for now ? Later on we can add abstraction if needed .There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep it. I see cases where depending on flavours of replication mode, we might want to add may be couple of more things - dynamic or static in nature. Since this has huge penetration in tests and other places, i feel we can keep this unless there is a strong reason not to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a good use case to keep
ReplicationOverridePolicy
at this point, we can add as needed as of now it just adds a level of indirectionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, will make the change.