[GitHub] [flink] dannycranmer commented on a diff in pull request #20542: [FLINK-28910][Connectors/hbase]Fix potential data deletion while updating HBase rows

[GitBox <gi...@apache.org>]

dannycranmer commented on code in PR #20542:
URL: https://github.com/apache/flink/pull/20542#discussion_r971658905


##########
flink-connectors/flink-connector-hbase-base/src/main/java/org/apache/flink/connector/hbase/sink/HBaseSinkFunction.java:
##########
@@ -76,6 +79,7 @@
     private transient ScheduledExecutorService executor;
     private transient ScheduledFuture scheduledFuture;
     private transient AtomicLong numPendingRequests;
+    private static Map<byte[], Mutation> mutationMap = new HashMap<>();

Review Comment:
   Why is this static? This means subtasks on the same JVM would all have access to, and try to flush the same data.



##########
flink-connectors/flink-connector-hbase-base/src/main/java/org/apache/flink/connector/hbase/sink/HBaseSinkFunction.java:
##########
@@ -213,6 +225,7 @@ public void close() throws Exception {
 
         if (mutator != null) {
             try {
+                flush();

Review Comment:
   While your thread safety looks ok (besides the `static` `Map`), generally speaking `flush()` on `close()` can cause issues. If the destination is down, the job might fail to stop. A better solution is to checkpoint the internal buffer. Longer term we can consider migrating to the Async Sink base.



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org