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
Add replication orchestration for a single shard #3533
Add replication orchestration for a single shard #3533
Changes from 5 commits
f6cb092
704a109
ed3fe28
f366918
ebb2b5e
531dedd
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.
Why does
IndexShard
specifically need to know aboutNRTReplicationEngine
can we simply return anEngine
reference instead?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.
@Poojita-Raj we basically expose whole engine here which we were trying to avoid in the first place #3533 (comment)
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.
@reta - Ah I see, I misunderstood. I've made the change to add the method updateSegments to Engine that by default throws an UnsupportedException, and which NRTReplicationEngine overrides to implement.
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.
Thanks @Poojita-Raj , the interface (like
ReplicationEngine
) would solve this ambiguity: if the engine does not implement it - no instance is returned.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.
Thanks @reta, I've gone ahead with the interface implementation. Let me know if there are any other callouts :)
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.
It seems like we are adding the method to be called by everyone but only supported for
NRTReplicationEngine
. May we could open up thegetEngine()
andSegmentReplicationTarget
could make the decision ifupdateSegments
should be called?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.
For now I'm still concerned about opening up access of engine beyond IndexShard
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.
Fair enouh, I think we can still do better than that:
Than IndexShard can do:
The main idea here: if something is not implemented, it should not be called. In this case, if engine does not support replication - the
getReplicationEngine()
returns empty and the caller won't be able to make a call.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.
Wondering if we really need a new Engine interface, does the current Engine interface not perform anything that has to do with replication already?
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.
@Bukhtawar the
Engine
is not exposed directly but performs everything needed, the goal here to expose only the small piece of engine functionality (ReplicationEngine) if the engine supports replication.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.
@reta I understand what the interface intends to do, what I have a concern on is the interface looks incomplete to me. I am not quite sure if
void updateSegments(final SegmentInfos infos, long seqNo) throws IOException;
is the only thing aReplicationEngine
should doThere 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.
@Bukhtawar 👍
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.
ReplicationEngine
is a regular Engine implementation and right now the only additional functionality is updating & refreshing on an externally provided SegmentInfos inNRTReplicationEngine
. @Bukhtawar what additional functionality do you think is missing? We could add a default implementation in Engine that throws aUnsupportedOperationException
and only implement it inNRTReplicationEngine
but I like the isolated interface.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 would revisit the
Engine
interface and see what can be extracted into a separateReplicationEngine
interface and decide on the split. Based on what you mention @mch2 we can also create a functional interface and call it aSegmentUpdateService
instead so that it appears less overloaded and performs the limited responsibility it specialises in/is tasked with.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.
EDIT: @Poojita-Raj pointed me to #3533 (comment) for more context on this change. Given that the only caller of
getReplicationEngine
is thefinalizeReplication
method, could we mitigate this by simply makinggetReplicationEngine
private, and then usingNRTReplicationEngine
directly as described below?i agree that the current
ReplicationEngine
interface seems out of place and incomplete. Why not useNRTReplicationEngine
directly?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.
Can we have multiple threads invoking this, in which case we would need it to be synchronized.
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.
Nope, it's only called by a single thread