mxm commented on code in PR #15151:
URL: https://github.com/apache/iceberg/pull/15151#discussion_r2815681390
##########
flink/v2.1/flink/src/main/java/org/apache/iceberg/flink/maintenance/operator/TriggerManagerCoordinator.java:
##########
@@ -76,8 +76,12 @@ private void registerLock(LockRegisterEvent
lockRegisterEvent) {
subtaskGateways().getSubtaskGateway(0).sendEvent(lock);
});
- PENDING_RELEASE_EVENTS.forEach(this::handleReleaseLock);
- PENDING_RELEASE_EVENTS.clear();
+ if (!PENDING_RELEASE_EVENTS.isEmpty()) {
+ synchronized (PENDING_RELEASE_EVENTS) {
+ PENDING_RELEASE_EVENTS.forEach(this::handleReleaseLock);
+ PENDING_RELEASE_EVENTS.clear();
+ }
+ }
Review Comment:
I think this is not correct. You need to move the check inside the
synchronized block.
##########
flink/v2.1/flink/src/main/java/org/apache/iceberg/flink/maintenance/operator/BaseCoordinator.java:
##########
Review Comment:
This can be a normal `ArrayList` now.
##########
flink/v2.1/flink/src/main/java/org/apache/iceberg/flink/maintenance/operator/BaseCoordinator.java:
##########
Review Comment:
Can we make this a regular map as well?
##########
flink/v2.1/flink/src/main/java/org/apache/iceberg/flink/maintenance/operator/BaseCoordinator.java:
##########
Review Comment:
Why not make the whole method `synchronized`? I think this makes things a
bit easier.
##########
flink/v2.1/flink/src/main/java/org/apache/iceberg/flink/maintenance/operator/BaseCoordinator.java:
##########
@@ -81,11 +81,21 @@ void handleReleaseLock(LockReleaseEvent lockReleaseEvent) {
lockReleaseEvent.lockId(),
lockReleaseEvent.timestamp());
} else {
Review Comment:
This block now gets repeated in the else `block` below. IMHO, we should just
make the whole method synchronized to avoid those premature optimizations which
make the code hard to read.
--
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]
