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
Refactor InternalEngine into abstract Engine and classes #9585
Conversation
What purpose does having an |
@rjernst I prefer the interface for a plugin or feature that may want to implement an engine from scratch without conforming to the That said, it's probably heavily personal preference, I think it's cleaner to leave it as an interface, but I'm happy to change it if people think otherwise :) |
* A special {@link RecoveryCounter} that flushes the engine when all | ||
* recoveries have completed | ||
*/ | ||
public final class FlushingRecoveryCounter extends RecoveryCounter { |
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.
do you have a use case for the non - flushing variant? if so I would fold the counter into this one. I think the extra flushing in the name is good to indicate it's not a simple counter.
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.
Yes, the non-flushing one is used for the ShadowEngine
bq. What purpose does having an Engine interface serve if there is a single AbstractEngine that everything inherits from? Could Engine be made an abstract class and this shared stuff moved there? I'm trying to understand why we need an extra layer of abstraction. +1 on removing the interface - I don't think we need this and it just adds complexity |
/** | ||
* Lock used inside of Engine implementations | ||
*/ | ||
public class EngineLock implements Releasable { |
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.
maybe this should be a general purpose class called ReleasableLock
?
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.
Okay, I'll rename this.
@s1monw pushed a couple of new commits to this, however, separating the packages between How do you feel about removing |
bq. How do you feel about removing engine.internal entirely and moving everything into engine? |
@s1monw pushed another commit that moves everything into |
/** | ||
* Releasable lock used inside of Engine implementations | ||
*/ | ||
public class ReleasableLock implements Releasable { |
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.
maybe move this to org.elasticsearch.common.util.concurrent
left minor comment - LGTM |
486732b
to
622d2c8
Compare
InternalEngine contains a number of inner classes that it uses, however, this makes the class overly large and hard to extend. In order to be able to easily add other Engines (such as the ShadowEngine), these helping methods have been extracted into an AbstractEngine class. The classes that were previously in `InternalEngine` have been moved to separate classes, which will allow for better unit testing as well. None of the functionality of InternalEngine has been changed, this is only refactoring. Note that this is a change I originally made on my shadow-replica branch, however it is easier to review piecemeal so I extracted it into a separate PR.
InternalEngine contains a number of inner classes that it uses, however,
this makes the class overly large and hard to extend. In order to be
able to easily add other Engines (such as the ShadowEngine), these
helping methods have been extracted into an AbstractEngine class. The
classes that were previously in
InternalEngine
have been moved toseparate classes, which will allow for better unit testing as well.
None of the functionality of InternalEngine has been changed, this is
only refactoring.
Note that this is a change I originally made on my shadow-replica
branch, however it is easier to review piecemeal so I extracted it into
a separate PR.