Add Index Operations to Translog from Engine #2977
Conversation
Signed-off-by: Rishikesh1159 <[email protected]>
Signed-off-by: Rishikesh1159 <[email protected]>
Signed-off-by: Rishikesh1159 <[email protected]>
|
Looks good to me! 🚀 My one nitpick/request would be to add javadocs to the new API in I'm stopping short of an approval since I'd like a fresh set of eyes on this 😄 |
Sure. I will update the new API with java docs |
| @@ -1075,6 +1059,44 @@ public IndexResult index(Index index) throws IOException { | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| public Engine.IndexResult addIndexOperationToTranslog(Index index) throws IOException { | |||
| try (Releasable ignored = versionMap.acquireLock(index.uid().bytes())) { | |||
There was a problem hiding this comment.
I believe you need to acquire readLock lock here (as all other public methods do):
try (ReleasableLock ignored = readLock.acquire()) {
....
}
Also, it probably makes sense to do basic checks like ensureOpen(); and assertIncomingSequenceNumber before adding anything to translog.
ensureOpen();
assert assertIncomingSequenceNumber(index.origin(), index.seqNo());
Another concern I have is that write operations like index and delete do update lastWriteNanos but this is not done here, is there a reason?
There was a problem hiding this comment.
I'd actually prefer to leave this method out until a segrep PR needs it. Nothing in this PR, except tests, call this method so it doesn't make sense to add at this point until we have a change that's actually using it. That will also concretely communicate why the method is needed to those not intimately working segrep; along with how to best structure the method.
Signed-off-by: Rishikesh1159 <[email protected]>
| @@ -1075,6 +1059,44 @@ public IndexResult index(Index index) throws IOException { | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| public Engine.IndexResult addIndexOperationToTranslog(Index index) throws IOException { | |||
| try (Releasable ignored = versionMap.acquireLock(index.uid().bytes())) { | |||
There was a problem hiding this comment.
I'd actually prefer to leave this method out until a segrep PR needs it. Nothing in this PR, except tests, call this method so it doesn't make sense to add at this point until we have a change that's actually using it. That will also concretely communicate why the method is needed to those not intimately working segrep; along with how to best structure the method.
The aim of these PRs was to front-load refactoring in existing classes that we've made in the If we leave out the @nknize any suggestions on how we could balance this better? |
We should merge #2904 behind a feature flag (e.g., |
|
@Rishikesh1159 Any updates on this? Are you still blocked? |
|
Closing this PR. Changes necessary are added in a different PR. |
Description
This PR will allow us to decouple adding Index operations to Lucene and adding Index operations to Translog. This would be useful for segment replication, as we will need to just add Index operation to replicas translog instead of performing full index operation. We will only use this method when segment replication is enabled and when we are on a replica node. The logic for checking if segment replication is enabled and is replica node will be added later in a different PR.
This is a part of the process of merging our feature branch - feature/segment-replication - back into main by re-PRing our changes from the feature branch.
The breakdown of the plan to merge to main is detailed here: #2926
For added context on segment replication - here's the design proposal #2229
Issues Resolved
#2926
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.