bkonold commented on a change in pull request #1385:
URL: https://github.com/apache/samza/pull/1385#discussion_r456765279
##########
File path:
samza-core/src/main/scala/org/apache/samza/storage/ContainerStorageManager.java
##########
@@ -604,6 +597,9 @@ private StorageEngine createStore(String storeName,
TaskName taskName, TaskModel
Map<String, StorageEngine> sideInputStores =
getSideInputStores(taskName);
Map<String, Set<SystemStreamPartition>> sideInputStoresToSSPs = new
HashMap<>();
+ CountDownLatch taskCountDownLatch = new CountDownLatch(1);
+ this.sideInputTaskLatches.put(taskName, taskCountDownLatch);
Review comment:
IMO this is safer as it eliminates sharing of objects between class
instances. E.g. in the shared case a task could mistakenly `countDown` the
latch more than once, which would be logically incorrect. The way it's written
now, that would not be possible.
I think it is also more readable; a dev can read through and understand the
latch within `TaskSideInputHandler` without needing to know the relationship to
CSM.
I don't see an obvious shortcoming to the approach I've taken, can you
explain if your preference is to use one latch and why?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
