Skip to content
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 NoOps to Lucene for failed delete operations #33217

Merged
merged 2 commits into from
Aug 30, 2018

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Aug 28, 2018

Today we add a NoOp to Lucene and translog if we fail to process an
indexing operation. However, we are only adding NoOps to translog for
delete operations. In order to have a complete history in Lucene, we
should add NoOps of failed delete operations to both Lucene and translog.

Relates #29530

Today we add a NoOp to Lucene and translog if we fail to process an
indexing operation. However, we are only adding NoOps to translog for
delete operations. In order to have a complete history in Lucene, we
should add a Noop to both Lucene and translog if we fail to apply a
delete.
@dnhatn dnhatn added >feature :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. labels Aug 28, 2018
@dnhatn dnhatn requested review from s1monw and jasontedor August 28, 2018 19:37
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@dnhatn
Copy link
Member Author

dnhatn commented Aug 28, 2018

@dnhatn
Copy link
Member Author

dnhatn commented Aug 28, 2018

@elasticmachine test this please.

@s1monw
Copy link
Contributor

s1monw commented Aug 29, 2018

technically I agree with this. But what is the scenario where a delete op fails? I don't necessarily understand how this can happen. Can you explain this a bit?

@dnhatn
Copy link
Member Author

dnhatn commented Aug 29, 2018

@s1monw To be honest, I am not sure if this ever happens. I made this change because:

  • We catch this failure in our current implementation but put NoOps into translog only
  • A delete with soft-deletes enabled is an index (update DV, then index a new tombstone document); thus deletes may fail like index operations.

@s1monw
Copy link
Contributor

s1monw commented Aug 29, 2018

I suggest we make this change for correctness and then open an issue to revisit this altogether. I think it's unnecessary we can't recover from any issue during a delete IMO

@dnhatn
Copy link
Member Author

dnhatn commented Aug 30, 2018

@s1monw I opened #33256. Can you please give this go? Thank you!

Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

LGTM given that we have a followup

@dnhatn
Copy link
Member Author

dnhatn commented Aug 30, 2018

Thanks @s1monw.

@dnhatn dnhatn merged commit 1326199 into elastic:ccr Aug 30, 2018
@dnhatn dnhatn deleted the noop-for-failed-deletes branch August 30, 2018 11:55
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Aug 30, 2018
Today we add a NoOp to Lucene and translog if we fail to process an
indexing operation. However, we are only adding NoOps to translog for
delete operations. In order to have a complete history in Lucene, we
should add NoOps of failed delete operations to both Lucene and translog.

Relates elastic#29530
dnhatn added a commit that referenced this pull request Aug 30, 2018
Today we add a NoOp to Lucene and translog if we fail to process an
indexing operation. However, we are only adding NoOps to translog for
delete operations. In order to have a complete history in Lucene, we
should add NoOps of failed delete operations to both Lucene and translog.

Relates #29530
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. >feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants