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

Make Translog an implementation detail of an Engine instead of required #31248

Closed
dakrone opened this issue Jun 11, 2018 · 6 comments
Closed
Labels
:Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. help wanted adoptme >refactoring

Comments

@dakrone
Copy link
Member

dakrone commented Jun 11, 2018

This is a followup to #31220 (comment) where it would be nice if we could make the abstract Engine class not expose information that made it seem like it has a Translog. This would include functions with Translog in the name, as well as perhaps using return values that aren't sub classes of Translog.

It would be nice if an engine consumer could use an engine without having to know about translog implementation details (or without the presence of a translog, if desired).

@dakrone dakrone added help wanted adoptme :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. >refactoring labels Jun 11, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@dakrone
Copy link
Member Author

dakrone commented Jun 11, 2018

Here's an initial list that would need renaming, removing, or reorganizing

  • isTranslogSyncNeeded -> isSyncNeeded
  • syncTranslog -> sync
  • ensureTranslogSynced -> ensureSynced
  • acquireTranslogRetentionLock -> acquireRetentionLock
  • newTranslogSnapshotFromMinSeqNo -> newSnapshotFromMinSeqNo
  • estimateTranslogOperationsFromMinSeq -> estimateOperationsFromMinSeq
  • getTranslogStats -> getStats
  • getTranslogLastWriteLocation -> getLastWriteLocation

Additionally, if we wanted to do this we'd have to have generic return values and eschew our use of Translog.Snapshot and Translog.Location

@jasontedor
Copy link
Member

I don't think this issue should be about renaming these methods but rather removing their need to be on the Engine public API? These methods simply do not make sense without translog in the name if they are doing something to the translog.

@dakrone
Copy link
Member Author

dakrone commented Jun 11, 2018

I agree, we don't have to keep all the methods, I editing the comment to say "renaming, removing, or reorganizing"

@ywelsch
Copy link
Contributor

ywelsch commented Jan 7, 2019

@dakrone Can this be closed now with #31220 merged?

@dakrone
Copy link
Member Author

dakrone commented Mar 7, 2019

I think this can be closed now.

@dakrone dakrone closed this as completed Mar 7, 2019
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. help wanted adoptme >refactoring
Projects
None yet
Development

No branches or pull requests

4 participants