Inject StorageProvider into PantheonController instances #259
Conversation
…them from RocksDB.
… RocksDB based storage.
|
|
||
| import java.io.Closeable; | ||
|
|
||
| public interface StorageProvider extends Closeable { |
There was a problem hiding this comment.
Instead of our own new interface could we use a pair of javax.inject.Provider instances? Do they need to be tied like this? This would make applying a DI framework later much easier. JSR-330 annotations have no tie to a specific DI framework.
https://docs.oracle.com/javaee/6/api/javax/inject/Provider.html
There was a problem hiding this comment.
The problem with having them supplied separately is that they will also have to be closed separately which is a problem because they will often use the same backend storage (RocksDB currently) and it's the actual storage backend that really gets closed.
There was a problem hiding this comment.
With a guice or dagger module we could aggregate the closables in the module. But we don't have that in this CL so this interface will work.
|
|
||
| public interface StorageProvider extends Closeable { | ||
|
|
||
| BlockchainStorage createBlockchainStorage(ProtocolSchedule<?> protocolSchedule); |
There was a problem hiding this comment.
Can we do this without the ProtocolSchedule argument? Is there a case where we don't know the protocol schedule when we are making the decisions about what kind of blockchain storage we provide? We could just curry a function taking that into the constructor and make it a provider. It's a bit of work on our end but when we go to DI the framework will wire it together.
There was a problem hiding this comment.
Unfortunately we don't have the ProtocolSchedule available at the time we want to create the StorageProvider. The issue is that you need a BlockHashFunction to load any blocks from storage and that depends on the ProtocolSchedule. We very likely want to remove that dependency which would mean removing the hash function from Block and BlockHeader - anyone wanting the hash would have to get the BlockHashFunction from the ProtocolSchedule and pass the BlockHeader to it. That's not a simple change to make and we would need to preserve the hash caching that BlockHeader currently provides internally as it's a fairly expensive operation and is performed quite a bit.
There was a problem hiding this comment.
Yes, it's hard to go halfway with a DI style without a framework. A lot of these issues would melt into the module (one for each consensus protocol) but without that framework we can't go halfway. So since we don't have a forma DI framework handling the injections for us this is the engineering compromise we will need to do.
|
|
||
| import java.io.Closeable; | ||
|
|
||
| public interface StorageProvider extends Closeable { |
There was a problem hiding this comment.
With a guice or dagger module we could aggregate the closables in the module. But we don't have that in this CL so this interface will work.
|
|
||
| public interface StorageProvider extends Closeable { | ||
|
|
||
| BlockchainStorage createBlockchainStorage(ProtocolSchedule<?> protocolSchedule); |
There was a problem hiding this comment.
Yes, it's hard to go halfway with a DI style without a framework. A lot of these issues would melt into the module (one for each consensus protocol) but without that framework we can't go halfway. So since we don't have a forma DI framework handling the injections for us this is the engineering compromise we will need to do.
PR description
Decouples PantheonController instances from RocksDB by injecting a
StorageProviderthat is used to create storage for blockchain and world state.