-
Notifications
You must be signed in to change notification settings - Fork 177
feat: remove base repl endpoint from hbase 2.x #3539
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
.../com/google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpoint.java
Outdated
Show resolved
Hide resolved
.../com/google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpoint.java
Show resolved
Hide resolved
.../com/google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpoint.java
Outdated
Show resolved
Hide resolved
.../com/google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpoint.java
Show resolved
Hide resolved
.../com/google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpoint.java
Show resolved
Hide resolved
.../com/google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpoint.java
Show resolved
Hide resolved
.../com/google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpoint.java
Outdated
Show resolved
Hide resolved
.../java/com/google/cloud/bigtable/hbase2_x/replication/configuration/CustomWALEntryConfig.java
Outdated
Show resolved
Hide resolved
.../google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpointTest.java
Outdated
Show resolved
Hide resolved
.../google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpointTest.java
Outdated
Show resolved
Hide resolved
35e3fc6 to
617034a
Compare
.../com/google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpoint.java
Show resolved
Hide resolved
.../com/google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpoint.java
Show resolved
Hide resolved
.../com/google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpoint.java
Outdated
Show resolved
Hide resolved
.../com/google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpoint.java
Outdated
Show resolved
Hide resolved
.../com/google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpoint.java
Show resolved
Hide resolved
.../google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpointTest.java
Show resolved
Hide resolved
| TestUtils.waitForReplication( | ||
| () -> { | ||
| // replicate Entries will be zero | ||
| return TestReplicationEndpoint.replicatedEntries.get() >= 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FILTERED_ROW_KEY is not passed under replicateContext
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then explain in the comment that FILTERED KEY will not be passed. A new engineer debugging this test should not need to figure that part out.
| "Number of cells , actual cells: " + hbaseResult.listCells(), | ||
| 2, | ||
| 2, | ||
| hbaseResult.listCells().size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| Result cbtResult = cbtTable.get(new Get(ROW_KEY).setMaxVersions()); | ||
| Result hbaseResult = hbaseTable.get(new Get(ROW_KEY).setMaxVersions()); | ||
| Assert.assertFalse(cbtResult.isEmpty()); | ||
| Assert.assertFalse(hbaseResult.isEmpty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required
| Result hbaseResult = hbaseTable.get(new Get(ROW_KEY).setMaxVersions()); | ||
| Assert.assertFalse(cbtResult.isEmpty()); | ||
| Assert.assertFalse(hbaseResult.isEmpty()); | ||
| Assert.assertEquals( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method seems weird, what are we validating ? The name should give me a hint, this is not an "assertEquals" method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| 1, | ||
| 1, | ||
| cbtResult.listCells().size()); | ||
| TestUtils.assertEquals( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
.../google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpointTest.java
Show resolved
Hide resolved
| "Number of cells , actual cells: " + hbaseResult.listCells(), | ||
| 2, | ||
| 2, | ||
| hbaseResult.listCells().size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed.
| Result cbtResult = cbtTable.get(new Get(ROW_KEY).setMaxVersions()); | ||
| Result hbaseResult = hbaseTable.get(new Get(ROW_KEY).setMaxVersions()); | ||
| Assert.assertFalse(cbtResult.isEmpty()); | ||
| Assert.assertFalse(hbaseResult.isEmpty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
| Result hbaseResult = hbaseTable.get(new Get(ROW_KEY).setMaxVersions()); | ||
| Assert.assertFalse(cbtResult.isEmpty()); | ||
| Assert.assertFalse(hbaseResult.isEmpty()); | ||
| Assert.assertEquals( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not calling your method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| TestUtils.assertEquals( | ||
| hbaseResult, | ||
| cbtResult, | ||
| HConstants.REPLICATION_SCOPE_GLOBAL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check it in test. Use assertEqual on cells.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
sushanb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed PR feedbacks
| "Number of cells , actual cells: " + hbaseResult.listCells(), | ||
| 2, | ||
| 2, | ||
| hbaseResult.listCells().size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| Result hbaseResult = hbaseTable.get(new Get(ROW_KEY).setMaxVersions()); | ||
| Assert.assertFalse(cbtResult.isEmpty()); | ||
| Assert.assertFalse(hbaseResult.isEmpty()); | ||
| Assert.assertEquals( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| 1, | ||
| 1, | ||
| cbtResult.listCells().size()); | ||
| TestUtils.assertEquals( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
.../google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpointTest.java
Show resolved
Hide resolved
| "Number of cells , actual cells: " + hbaseResult.listCells(), | ||
| 2, | ||
| 2, | ||
| hbaseResult.listCells().size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed.
| Result cbtResult = cbtTable.get(new Get(ROW_KEY).setMaxVersions()); | ||
| Result hbaseResult = hbaseTable.get(new Get(ROW_KEY).setMaxVersions()); | ||
| Assert.assertFalse(cbtResult.isEmpty()); | ||
| Assert.assertFalse(hbaseResult.isEmpty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
| TestUtils.assertEquals( | ||
| hbaseResult, | ||
| cbtResult, | ||
| HConstants.REPLICATION_SCOPE_GLOBAL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| Result hbaseResult = hbaseTable.get(new Get(ROW_KEY).setMaxVersions()); | ||
| Assert.assertFalse(cbtResult.isEmpty()); | ||
| Assert.assertFalse(hbaseResult.isEmpty()); | ||
| Assert.assertEquals( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
a33222a to
a938eac
Compare
| TestUtils.waitForReplication( | ||
| () -> { | ||
| // replicate Entries will be zero | ||
| return TestReplicationEndpoint.replicatedEntries.get() >= 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then explain in the comment that FILTERED KEY will not be passed. A new engineer debugging this test should not need to figure that part out.
.../google/cloud/bigtable/hbase1_x/replication/HbaseToCloudBigtableReplicationEndpointTest.java
Show resolved
Hide resolved
.../google/cloud/bigtable/hbase1_x/replication/HbaseToCloudBigtableReplicationEndpointTest.java
Show resolved
Hide resolved
.../google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpointTest.java
Show resolved
Hide resolved
.../google/cloud/bigtable/hbase2_x/replication/HbaseToCloudBigtableReplicationEndpointTest.java
Outdated
Show resolved
Hide resolved
🤖 I have created a release *beep* *boop* --- ## [2.1.0](v2.0.0...v2.1.0) (2022-04-01) ### Features * [hase-cbt-replication]Add a metrics for hbase<>bigtable timestamp overflow ([#3540](#3540)) ([9c73318](9c73318)) * Adding a detailed readme for HBase replication support. ([#3527](#3527)) ([b406ac5](b406ac5)) * enable dry-run mode for HBase to Cloud Bigtable replication ([#3532](#3532)) ([164738b](164738b)) * Enable HBase to Cloud Bigtable replication ([#3510](#3510)) ([68d4a01](68d4a01)) * flag a put in future timestamp and record a metrics ([#3534](#3534)) ([7dd7653](7dd7653)) * remove base repl endpoint from hbase 2.x ([#3539](#3539)) ([f748c70](f748c70)) ### Bug Fixes * [hbase-cbt-replication] make hbase2.x endpoint context private ([#3559](#3559)) ([5ec3b18](5ec3b18)) * consistent behavior of customwalentry filter across hbase1.x and hbase2.x ([#3562](#3562)) ([f70b0cb](f70b0cb)) * license and notice files for all the shaded artifacts ([#3565](#3565)) ([86a5643](86a5643)) * minor documentation update ([#3572](#3572)) ([68db6c2](68db6c2)) ### Documentation * fix typos in README and add --region option ([#3520](#3520)) ([c189d46](c189d46)) ### Dependencies * update dependency ch.qos.logback:logback-classic to v1.2.11 ([#3538](#3538)) ([92e8b63](92e8b63)) * update dependency com.google.cloud:google-cloud-bigtable-emulator to v0.142.3 ([#3528](#3528)) ([33a8eba](33a8eba)) * update dependency com.google.cloud:google-cloud-bigtable-emulator to v0.143.0 ([#3543](#3543)) ([2d57f14](2d57f14)) * update dependency net.bytebuddy:byte-buddy to v1.12.8 ([#3502](#3502)) ([77ae2f1](77ae2f1)) * update jmh.version to v1.35 ([#3561](#3561)) ([188738d](188738d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> ☕️