[FLINK-32139][connector/hbase] Using strongly increasing nanosecond timestamp and DeleteColumn type to fix data accidental deletion and non deletion issues when sink hbase. #22612

lzshlzsh · 2023-05-20T15:13:49Z

What is the purpose of the change

Using strongly increasing nanosecond timestamp and DeleteColumn type to fix data accidental deletion and non deletion issues when sink upsert data to hbase.

https://issues.apache.org/jira/browse/FLINK-32139

Brief change log

Add a class HBaseTimestampGenerator which generates strongly increasing timestamp in nanoseconds to set to hbase mutation.
HBaseSerde createPutMutation and createDeleteMutation use muation with timestamp.
Use delete.addColumns instead of delete.addColumn, the first hbase client api use DeleteColumn key type and the second use Delete key type.

Verifying this change

Added test that validates that HBaseTimestampGenerator generates strongly increasing timestamp.
Online test described in #32139

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): no
The public API, i.e., is any changed class annotated with @Public(Evolving): no
The serializers: no
The runtime per-record code paths (performance sensitive): no
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
The S3 file system connector: no

Documentation

Does this pull request introduce a new feature? no

flinkbot · 2023-05-20T15:17:24Z

CI report:

fe2ef22 UNKNOWN
8d3b74f Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

...-hbase-base/src/main/java/org/apache/flink/connector/hbase/util/HBaseTimestampGenerator.java

kylemeow · 2023-05-22T04:43:30Z

...se-base/src/test/java/org/apache/flink/connector/hbase/util/HBaseTimestampGeneratorTest.java

+        for (int i = 0; i < 100_000_000; i++) {
+            long now = timestampGenerator.get();
+            if (lastTimestamp > 0) {
+                assertTrue(lastTimestamp < now);


Better to use 'now > lastTimestamp' as assertion condition to explicitly show that timestamp is increasing

good advice

kylemeow · 2023-05-22T04:43:47Z

...se-base/src/test/java/org/apache/flink/connector/hbase/util/HBaseTimestampGeneratorTest.java

+        }
+        final long realNow = timestampGenerator.getCurrentSystemTimeNano();
+        final long diff = lastTimestamp - realNow;
+        assertTrue("Should no timestamp diff in 100 billion tests", diff <= 0);


billion → million

kylemeow · 2023-05-22T04:43:57Z

...se-base/src/test/java/org/apache/flink/connector/hbase/util/HBaseTimestampGeneratorTest.java

+        final long diff = lastTimestamp - realNow;
+        assertTrue("Should no timestamp diff in 100 billion tests", diff <= 0);
+        LOG.info(
+                "timestamp diff of 100 billion tests in nanosecond: {} (use {}, real {})",


billion → million

kylemeow · 2023-05-22T04:44:43Z

...se-base/src/test/java/org/apache/flink/connector/hbase/util/HBaseTimestampGeneratorTest.java

+            lastTimestamp = now;
+        }
+        final long realNow = timestampGenerator.getCurrentSystemTimeNano();
+        final long diff = lastTimestamp - realNow;


Better to use 'realNow - lastTimestamp' to explicitly show that the timestamp is increasing

…DeleteColumn type to fix data accidental deletion and non deletion issues when sink hbase.

kylemeow · 2023-05-22T04:48:14Z

Also, it is recommended that you add test cases to verify this HBase data loss issue is completely fixed with this solution.

MartijnVisser · 2023-05-22T13:41:38Z

...se-base/src/test/java/org/apache/flink/connector/hbase/util/HBaseTimestampGeneratorTest.java

+    public void testStronglyIncreasingTimestampGenerator() {
+        HBaseTimestampGenerator timestampGenerator = HBaseTimestampGenerator.stronglyIncreasing();
+        long lastTimestamp = 0;
+        for (int i = 0; i < 100_000_000; i++) {


Please have a look at https://flink.apache.org/how-to-contribute/code-style-and-quality-common/#7-testing - Having a test run 100 million times isn't an effective test

Please have a look at https://flink.apache.org/how-to-contribute/code-style-and-quality-common/#7-testing - Having a test run 100 million times isn't an effective test

@MartijnVisser
Thank you for your review and valuable feedback. I think I may need to use Junit5 to write test cases, and change 100 million times tests to one times。Can you give more suggestions on how to write this test?

Actually, I have another question. How to write test cases for the probability of errors occurring in this data loss situation, as we cannot assert a definite state ?

ferenc-csaky

The solution logic itself LGTM. Agree what other reviewers mentions about the tests. Added 1 more comment.

ferenc-csaky · 2023-05-24T11:42:12Z

...-hbase-base/src/main/java/org/apache/flink/connector/hbase/util/HBaseTimestampGenerator.java

+package org.apache.flink.connector.hbase.util;
+
+/** Generate timestamp for HBase mutation. */
+public abstract class HBaseTimestampGenerator {


Is there a chance we will have another implementation for this class in the future? Even if yes, I think we can introduce the extra abstraction layer when it will be necessary. For now, a simple class, e.g. StronglyIncreasingTsGenerator with 1 public method that returns the ts would be enough, resulting a simpler and cleaner solution.

Is there a chance we will have another implementation for this class in the future? Even if yes, I think we can introduce the extra abstraction layer when it will be necessary. For now, a simple class, e.g. StronglyIncreasingTsGenerator with 1 public method that returns the ts would be enough, resulting a simpler and cleaner solution.

Thank you for your feedback. I will make some modifications according to your suggestion.

lzshlzsh · 2023-05-24T16:06:04Z

@flinkbot run azure

ferenc-csaky

LGTM, thank you for incorporating the requested changes!

1996fanrui · 2023-06-06T07:08:57Z

Hi @MartijnVisser , thanks for the reviewing and merging.

A friendly reminder: Please note the squish commit before merging and the commit message should start with [FLINK-JIRA_ID], thanks~

MartijnVisser · 2023-06-06T07:09:53Z

A friendly reminder: Please note the squish commit before merging and the commit message should start with [FLINK-JIRA_ID], thanks~

Hi @1996fanrui - Yeah I made a mistake, there's unfortunately nothing I can do to repair that situation :(

kylemeow reviewed May 22, 2023

View reviewed changes

[connector/hbase] Using strongly increasing nanosecond timestamp and …

fe2ef22

…DeleteColumn type to fix data accidental deletion and non deletion issues when sink hbase.

lzshlzsh force-pushed the dev-fix-hbase-sink-delete-problem branch from a234181 to fe2ef22 Compare May 22, 2023 04:45

address reveiw comment

a7a1648

MartijnVisser reviewed May 22, 2023

View reviewed changes

ferenc-csaky reviewed May 24, 2023

View reviewed changes

address review comment

8d3b74f

ferenc-csaky approved these changes May 27, 2023

View reviewed changes

MartijnVisser merged commit ebfeb32 into apache:master Jun 5, 2023

kylemeow mentioned this pull request Jun 26, 2023

[FLINK-28910][Connectors/hbase]Fix potential data deletion while updating HBase rows #20542

Closed

flinkbot added the component=Connectors/HBase label Apr 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-32139][connector/hbase] Using strongly increasing nanosecond timestamp and DeleteColumn type to fix data accidental deletion and non deletion issues when sink hbase. #22612

[FLINK-32139][connector/hbase] Using strongly increasing nanosecond timestamp and DeleteColumn type to fix data accidental deletion and non deletion issues when sink hbase. #22612

lzshlzsh commented May 20, 2023 •

edited

Loading

flinkbot commented May 20, 2023 •

edited

Loading

kylemeow May 22, 2023

lzshlzsh May 22, 2023

kylemeow May 22, 2023

lzshlzsh May 22, 2023

kylemeow May 22, 2023

lzshlzsh May 22, 2023

kylemeow May 22, 2023

lzshlzsh May 22, 2023

kylemeow commented May 22, 2023

MartijnVisser May 22, 2023 •

edited

Loading

lzshlzsh May 23, 2023 •

edited

Loading

ferenc-csaky left a comment

ferenc-csaky May 24, 2023

lzshlzsh May 24, 2023 •

edited

Loading

lzshlzsh commented May 24, 2023

ferenc-csaky left a comment

1996fanrui commented Jun 6, 2023

MartijnVisser commented Jun 6, 2023

[FLINK-32139][connector/hbase] Using strongly increasing nanosecond timestamp and DeleteColumn type to fix data accidental deletion and non deletion issues when sink hbase. #22612

[FLINK-32139][connector/hbase] Using strongly increasing nanosecond timestamp and DeleteColumn type to fix data accidental deletion and non deletion issues when sink hbase. #22612

Conversation

lzshlzsh commented May 20, 2023 • edited Loading

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented May 20, 2023 • edited Loading

CI report:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylemeow commented May 22, 2023

MartijnVisser May 22, 2023 • edited Loading

Choose a reason for hiding this comment

lzshlzsh May 23, 2023 • edited Loading

Choose a reason for hiding this comment

ferenc-csaky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzshlzsh May 24, 2023 • edited Loading

Choose a reason for hiding this comment

lzshlzsh commented May 24, 2023

ferenc-csaky left a comment

Choose a reason for hiding this comment

1996fanrui commented Jun 6, 2023

MartijnVisser commented Jun 6, 2023

lzshlzsh commented May 20, 2023 •

edited

Loading

flinkbot commented May 20, 2023 •

edited

Loading

MartijnVisser May 22, 2023 •

edited

Loading

lzshlzsh May 23, 2023 •

edited

Loading

lzshlzsh May 24, 2023 •

edited

Loading