StefanRRichter commented on a change in pull request #7674: [FLINK-10043]
[State Backends] Refactor RocksDBKeyedStateBackend object
construction/initialization/restore code
URL: https://github.com/apache/flink/pull/7674#discussion_r257647534
##########
File path:
flink-state-backends/flink-statebackend-rocksdb/src/test/java/org/apache/flink/contrib/streaming/state/RocksDBStateBackendTest.java
##########
@@ -191,8 +189,9 @@ public void setupRocksKeyedStateBackend() throws Exception
{
allCreatedCloseables = new ArrayList<>();
- keyedStateBackend.db = spy(keyedStateBackend.db);
- keyedStateBackend.initializeSnapshotStrategy(null);
+ keyedStateBackend.setDb(spy(keyedStateBackend.getDb()));
Review comment:
This test seems to be the only reason why we have to introduce all the
`setDb(...)` methods in different classes and have to move away from `final`. I
think it would be better to keep the `final` and eliminate the need for this
setter. From what I can see, this test is doing too much to create a
`RocksDBKeyedStateBackend` because before the code was not very testable.
However, I think with your changes, the code could just either use the builder
(via the new constructor that takes the injected db) or even the contructor of
`RocksDBKeyedStateBackend` directly to pass in the db that you provide with the
setter here. There should be no longer a need to go all the way through the
`RocksDBStateBackend`. This would simplify the test and avoid all the
modifications that are introduced just for this test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
