-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Pass transient extra data on new commits #123175
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
Pass transient extra data on new commits #123175
Conversation
Relates ES-10718
181b2e7 to
6c653fc
Compare
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
…pass-translog-number
arteam
left a 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.
LGTM!
server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java
Outdated
Show resolved
Hide resolved
henningandersen
left a 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.
A smaller suggestion.
server/src/main/java/org/elasticsearch/index/engine/Engine.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java
Outdated
Show resolved
Hide resolved
…pass-translog-number
…pass-translog-number
…pass-translog-number
…pass-translog-number
…pass-translog-number
…pass-translog-number
…pass-translog-number
…pass-translog-number
kingherc
left a 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.
Incorporated your feedback. Feel free to review again!
server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/engine/Engine.java
Outdated
Show resolved
Hide resolved
fcofdez
left a 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.
LGTM
| long primaryTerm, | ||
| IndexCommitRef indexCommitRef, | ||
| Set<String> additionalFiles, | ||
| Object enginePreCommitData |
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.
now that I'm reading this again, I think that the name is misleading? this data was acquired after the commit was created.
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.
@henningandersen suggested the name, but I see also the slight confusion. I had originally suggested "transient commit data". Would that be better @fcofdez ? I welcome possible naming suggestions!
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.
(fyi "pre" I think meant "before the commit is exposed". but I see the slight confusion.)
|
Closing in favor of a different approach. |
Relates ES-10718