advancedxy commented on code in PR #510:
URL: https://github.com/apache/incubator-uniffle/pull/510#discussion_r1089880578
##########
server/src/main/java/org/apache/uniffle/server/ShuffleFlushManager.java:
##########
@@ -137,6 +113,14 @@ public void run() {
thread.start();
}
+ protected void startEventProcesser() {
+ // the thread for flush data
+ Thread processEventThread = new Thread(() -> processEvents());
Review Comment:
In java8, method reference is preferred, such as: `new
Thread(this::processEvents)`
##########
server/src/test/java/org/apache/uniffle/server/buffer/ShuffleBufferManagerTest.java:
##########
@@ -567,7 +568,25 @@ public void shuffleFlushThreshold() throws Exception {
StorageManager storageManager =
StorageManagerFactory.getInstance().createStorageManager(conf);
ShuffleFlushManager shuffleFlushManager = new ShuffleFlushManager(conf,
- "serverId", mockShuffleServer, storageManager);
+ "serverId", mockShuffleServer, storageManager) {
+
+ @Override
+ protected void startEventProcesser() {
Review Comment:
I don't think we should override `startEventProcesser`?
The `processEvents` could be override with your previous change to the
`ShuffleBufferManager`.
##########
server/src/test/java/org/apache/uniffle/server/buffer/ShuffleBufferManagerTest.java:
##########
@@ -567,7 +568,25 @@ public void shuffleFlushThreshold() throws Exception {
StorageManager storageManager =
StorageManagerFactory.getInstance().createStorageManager(conf);
ShuffleFlushManager shuffleFlushManager = new ShuffleFlushManager(conf,
- "serverId", mockShuffleServer, storageManager);
+ "serverId", mockShuffleServer, storageManager) {
Review Comment:
I believe create an anonymous subclass is generally avoided in practice.
Better way would be creating a `TestShuffleFlushManager` class in a new file
or as a private inner class.
##########
server/src/main/java/org/apache/uniffle/server/ShuffleFlushManager.java:
##########
@@ -145,6 +129,31 @@ public void addToFlushQueue(ShuffleDataFlushEvent event) {
}
}
+ public void processEvents() {
Review Comment:
this could be protect?
##########
server/src/test/java/org/apache/uniffle/server/buffer/ShuffleBufferManagerTest.java:
##########
@@ -567,7 +568,25 @@ public void shuffleFlushThreshold() throws Exception {
StorageManager storageManager =
StorageManagerFactory.getInstance().createStorageManager(conf);
ShuffleFlushManager shuffleFlushManager = new ShuffleFlushManager(conf,
- "serverId", mockShuffleServer, storageManager);
+ "serverId", mockShuffleServer, storageManager) {
+
+ @Override
+ protected void startEventProcesser() {
+ // do nothing
+ }
+
+ @Override
+ public void processEvents() {
Review Comment:
`processEvents` in the main file is simple enough, it can be override. But
we should override it with similar logic with the origin one.
This override seems to be quite different.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
