-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: Move out concrete working implementation and add working modes #307
base: main
Are you sure you want to change the base?
feat: Move out concrete working implementation and add working modes #307
Conversation
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #307 +/- ##
============================================
+ Coverage 99.62% 99.63% +0.01%
- Complexity 288 300 +12
============================================
Files 57 61 +4
Lines 1060 1101 +41
Branches 78 80 +2
============================================
+ Hits 1056 1097 +41
Misses 3 3
Partials 1 1
|
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.
I like the new structural changes. Try out the thread management changes commented on and see if you can utilize that ExecutorService API.
|
||
/** | ||
* Constructs a new {@code PublisherModeHandler} with the specified block stream configuration and publisher client. | ||
* |
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.
The build is showing: warning: no @param for publishStreamGrpcClient
. You just need to add it to the javadoc here
simulatorModeHandler = new ConsumerModeHandler(blockStreamConfig); | ||
} else { | ||
simulatorModeHandler = new CombinedModeHandler(blockStreamConfig); | ||
} |
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.
You might leverage a switch statement here like:
switch (simulatorMode) {
case PUBLISHER:
// code
break;
case CONSUMER:
// code
break;
default:
// code
break;
}
@@ -37,9 +37,11 @@ | |||
*/ | |||
public class PublishStreamGrpcClientImpl implements PublishStreamGrpcClient { | |||
|
|||
private final BlockStreamServiceGrpc.BlockStreamServiceStub stub; | |||
private final StreamObserver<PublishStreamRequest> requestStreamObserver; | |||
private BlockStreamServiceGrpc.BlockStreamServiceStub stub; |
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.
I think you can move this declaration of stub
to the init()
method (doesn't need to be a member variable).
@@ -24,6 +24,11 @@ | |||
* The PublishStreamGrpcClient interface provides the methods to stream the block and block item. | |||
*/ | |||
public interface PublishStreamGrpcClient { |
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.
I like the new init()
and shutdown()
methods 👍
* @param blockStreamConfig the configuration data for managing block streams | ||
*/ | ||
public CombinedModeHandler(@NonNull final BlockStreamConfig blockStreamConfig) { | ||
requireNonNull(blockStreamConfig); |
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.
Does @NonNull
in the parameter section give you the same safeguards as requireNonNull()
? You might be able to remove that line ^^
private final StreamingMode streamingMode; | ||
private final int delayBetweenBlockItems; | ||
private final int millisecondsPerBlock; | ||
private final int NANOS_PER_MILLI = 1_000_000; |
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.
You could promote this helpful math shortcut ^^ to the simulator Constants class
@NonNull final BlockStreamConfig blockStreamConfig, | ||
@NonNull PublishStreamGrpcClient publishStreamGrpcClient) { | ||
requireNonNull(blockStreamConfig); | ||
requireNonNull(publishStreamGrpcClient); |
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.
I think you can remove these requireNonNull()
checks give the @NonNull
annotations
@@ -116,16 +107,19 @@ public static void teardown() { | |||
* | |||
* @return a configured {@link GenericContainer} instance for the Block Node server | |||
*/ | |||
public static GenericContainer<?> getConfiguration() { | |||
protected static GenericContainer<?> getContainer() { |
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.
Perhaps change the name of this method to createContainer()
@@ -45,13 +45,13 @@ public NegativeServerAvailabilityTests() {} | |||
* Clean up method executed after each test. | |||
* | |||
* <p>This method stops the running container, resets the container configuration by retrieving | |||
* a new one through {@link BaseSuite#getConfiguration()}, and then starts the Block Node | |||
* a new one through {@link BaseSuite#getContainer()} ()}, and then starts the Block Node | |||
* container again. | |||
*/ | |||
@AfterEach | |||
public void cleanUp() { |
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 change this method name to restartContainer()
assertFalse(savedBlocksFolderAfter.isEmpty()); | ||
assertTrue(savedBlocksCountAfter > savedBlocksCountBefore); | ||
} | ||
|
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.
Rather than spin up new Threads individually (expensive). Consider using private ExecutorService executorService = Executors.newFixedThreadPool(8)
and managing simulator instances from that API with something like: executorService.execute(blockStreamSimulatorAppInstance::start);
. The ExecutorService
gives you a number of thread lifecycle management methods for free.
Description:
This PR aims mainly to make the maintanence and using of simulator (as test driver) easier. It moves away the implementation of the working modes and exposes only abstractions. So that all concrete implementations can be refactored (if needed), without the main application, knowing or caring about it.
Main simulator app should only be responsible for starting the processes and nothing more. Wherther the simulator will work in publisher or consumer mode, or both is not responsibility of the main class.
Also adds E2E test for data persistence.
Related issue(s):
Fixes #239
Notes for reviewer:
Checklist