Remove duplicate init code from PantheonController instances #1305
Conversation
…sAwarePantheonController so all the common parts of construction are shared.
…h settings in the genesis config which --network DEV handles so no need for an additional boolean devMode flag.
…ving a PantheonControllerBuilder that called into ConsensusAwarePantheonControllerBuilder. Actually use the builder pattern for the new builder.
| import org.apache.logging.log4j.LogManager; | ||
| import org.apache.logging.log4j.Logger; | ||
|
|
||
| public abstract class PantheonControllerBuilder<C> { |
There was a problem hiding this comment.
The generic C is an impediment to understanding what it really is. The Google Style guide section 5.2.8 allows for longer type names as long as it ends with a T. I think we should adopt that pattern and replace C with ConsensusContextT, which should improve the readability.
There was a problem hiding this comment.
We're pretty consistent in using C to indicate the consensus context across the code base. Changing it to a longer name would add quite a lot of noise. It's also an invasive enough change that we should do it in a separate PR if we're going to.
There was a problem hiding this comment.
I agree it should not be done in this PR, it is quite invasive.
But unless you already know how consensus state objects are used and passed around C may as well be I, X, or T. It is present in broad swaths of the code but what it actually means is limited to very few locations that are hard to navigate to unless you already know exactly what it does.
| private final List<Runnable> shutdownActions = new ArrayList<>(); | ||
| private RocksDbConfiguration rocksdDbConfiguration; | ||
|
|
||
| public static PantheonControllerBuilder<?> fromGenesisConfig( |
There was a problem hiding this comment.
A builder with constructor parameters feels a bit off. Perhaps these two fromGenesisConfig methods could be moved to a PantheonControllerBuilderFactory and this class could be AbstractPantheonControllerBuilder. The subclasses also don't add any builder functionality but are actually more of a strategy object for a generic factory.
Could this builder be a very simple builder that just holds the fields, and then in the build method delegates to the strategy/factory objects specific to the consensus types? As late as possible? Or perhaps keep the old class names and add a .fromBuilder method that will return an appropriate controller or throw an exception?
There was a problem hiding this comment.
Well it's not actually a constructor param - it's a static factory method to create the right type of builder based on the config. I've moved it to a separate PantheonControllerBuilderFactory class since that's essentially what PantheonCommand wants. PantheonCommandTest wants to inject a mock PantheonControllerBuilder and was using a Function<EthNetworkConfig, PantheonControllerBuilder> but can now use a mock PantheonControllerBuilderFactory.
Whether it's a static method or a separate class the idea of having a builder factory displeases me greatly but I can't see any real way around it.
The key challenge in all of this is getting Java's generics to play nice. We need the genesis config before we can decide what generic type is required but if the builder isn't generic then it when it creates the ProtocolSchedule it has to declare it as ProtocolSchedule<?> but then it doesn't have the matching consensus context type the subclasses need.
…ilder for consistency.
| import org.apache.logging.log4j.LogManager; | ||
| import org.apache.logging.log4j.Logger; | ||
|
|
||
| public abstract class PantheonControllerBuilder<C> { |
There was a problem hiding this comment.
I agree it should not be done in this PR, it is quite invasive.
But unless you already know how consensus state objects are used and passed around C may as well be I, X, or T. It is present in broad swaths of the code but what it actually means is limited to very few locations that are hard to navigate to unless you already know exactly what it does.
| return this; | ||
| } | ||
|
|
||
| public PantheonController<C> build() throws IOException { |
There was a problem hiding this comment.
What do you think about moving this class inside PantheonController and calling it PantheonController.Builder? It would be more consistent with our other builders. What complicates it is the fact that the Builder class is subclassed by the other controllers. But to typical users that would be transparent.
Either way, your call.
| final GenesisConfigOptions configOptions = genesisConfig.getConfigOptions(); | ||
| final PantheonControllerBuilder<?> builder; | ||
|
|
||
| if (configOptions.isEthHash()) { |
There was a problem hiding this comment.
Not for this PR, but we should think of a way to make this pluggable without prior knowledge of all the controller builders.
There was a problem hiding this comment.
Absolutely agreed. I can see a few places that pluggability is starting to emerge and would make this setup code cleaner (creating the storage provider being a prime example).
…Eng#1305) Moves init code into a separate builder instead of a static init method, with common code in an abstract base class and subclasses of the builder for each of the consensus variants.
…Eng#1305) Moves init code into a separate builder instead of a static init method, with common code in an abstract base class and subclasses of the builder for each of the consensus variants.
PR description
Introduce a
ConsensusAwarePantheonControllerBuilderwith subclasses for each of our consensus algorithms which is responsible for creating thePantheonController. This allows the common init code to be shared with consensus specific subclasses only overriding parts they actually need to change.