feat: add synchronous MirroringConnection to 2.x #109

mwalkiewicz · 2021-10-22T11:17:33Z

This change is

prawilny

Looks okay, I left some nitpicks.

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mwalkiewicz)

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringTable.java, line 101 at r1 (raw file):

      };

  protected final Table primaryTable;

I think it's a leftover from some WIP version.

bigtable-hbase-mirroring-client-2.x-parent/bigtable-hbase-mirroring-client-2.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase2_x/MirroringTable.java, line 30 at r1 (raw file):

import org.apache.hadoop.hbase.client.TableDescriptor;

public class MirroringTable extends com.google.cloud.bigtable.mirroring.hbase1_x.MirroringTable

Looks okay. Didn't we want to use 2.x API underneath, though?

mwalkiewicz

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @prawilny)

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringTable.java, line 101 at r1 (raw file):

Previously, prawilny (Adam Czajkowski) wrote…

I think it's a leftover from some WIP version.

No, it is used in the second file in this PR, in getDescriptor() method.

bigtable-hbase-mirroring-client-2.x-parent/bigtable-hbase-mirroring-client-2.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase2_x/MirroringTable.java, line 30 at r1 (raw file):

Previously, prawilny (Adam Czajkowski) wrote…

Looks okay. Didn't we want to use 2.x API underneath, though?

You mean rewrite those ops using 2.x async connection? I think that it is too much work and it is not worth it.

The base branch was changed.

prawilny

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mwalkiewicz)

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringConnection.java, line 70 at r2 (raw file):

  protected final boolean performWritesConcurrently;
  private final boolean waitForSecondaryWrites;

Some assertion that we don't use the useless configuration of options?

prawilny

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @mwalkiewicz)

prawilny

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mwalkiewicz)

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringTable.java, line 766 at r2 (raw file):

    Object[] results = new Object[1];
    try {
      batchWithSpan(Collections.singletonList(operation), results);

We're don't wrap it in SpanFactory.wrapPrimaryOperation. You omitted it deliberately, right?

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringTable.java, line 831 at r2 (raw file):

      this.mirroringTracer.spanFactory.wrapPrimaryOperation(primaryOperation, HBaseOperation.BATCH);
    } catch (RetriesExhaustedWithDetailsException e) {
      primaryBatchData.setException(e);

Why is RetriesExhausetedWithDetailsException treated differently?

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringTable.java, line 847 at r2 (raw file):

      } catch (ExecutionException e) {
        assert false;
        throw new IllegalStateException("secondaryResult thrown unexpected exception.");

micronit: "threw an"

prawilny

I didn't remember the code so I reread it (I'm sorry for the mess in the history). Left some questions (to make sure you didn't miss them). Consider it approved after looking at those two nitty remarks.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mwalkiewicz)

mwalkiewicz

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mwalkiewicz and @prawilny)

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringTable.java, line 831 at r2 (raw file):

Previously, prawilny (Adam Czajkowski) wrote…

Why is RetriesExhausetedWithDetailsException treated differently?

Done.

… review comments (googleapis#3347) * chore: revert review comments * feat: add MirroringOperationException exception markers (#125) * feat: concurrent writes in MirroringBufferedMutator (#80) * refactor: implement multiple argument operations on MirroringAsyncTable with specific operations rather than batch() (#75) * feat: implement MirroringAsyncTable#getName() (#132) * feat: use Logger rather than stdout in DefaultMismatchDetector (#128) * feat: synchronous writes (#88) * fix: implement heapSize method for RowCell (#111) * feat: FlowController accounts for memory usage (#137) * refactor: remove Configuration as a base of MirroringConfiguration (#127) * feat: MirroringAsyncBufferedMutator (#81) * refactor: rename WRITE_MISMATCH to SECONDARY_WRITE_ERROR (#138) * fix: BufferedMutator close() waits for all secondary flushes to finish (#110) * feat: 2.x reads sampling (#114) * refactor: make MirroringResultScanner synchronize on itself rather than MirroringTable (#134) * ConcurrentBufferedMutator integration tests (#135) * feat: add synchronous MirroringConnection to 2.x (#109) * fix: MirroringConnection in 2.x failed to compile (#139) * fix: fix BufferedMutator ITs (#140) * feat: run 1.x integration tests on MirroringConnection etc. from 2.x (#108) * feat: 2.x - rewrite Increment and Append as Put in batch (#116) * fix: fix build (#142) * refactor: minor fixes after review (#117) * feat: MirroringAsyncTable#getScanner() (#58) * test: 2.x integration tests (#112) * feat: implement MirroringAsyncBufferedMutatorBuilder (#144) * feat: log rows and values in DefaultMismatchDetector (#129) * fix: ITs - add expected parameter to MismatchDetectors (#153) * fix: force Append and Increment to return results and discard that result before returning it to user (#136) * fix: review fixes in utils * fix: review fixes in BufferedMutator * fix: review fixes in Faillog * fix: fixed reference counting * fix: review fixes in FlowController * fix: review fixes in metrics * fix: review fixes in verification * fix: Review fixes in MirroringTable * fix: review fixes in HBase 2.x client * fix: fixes in ITs * feat: MirrorinAsyncTable: scan(), scanAll() (#131) * fix: review fixes in tests * feat: MirroringConnection: timeout in close() and abort() (#133) * feat: better mismatch detection of scan results (#130) * feat: quickstart (#105) * fix: 2.x scan ITs (#158) * fix: DefaultMismatchDetector tests (#157) * fix: ConcurrentBufferedMutator waits for both flushes to finish before closing (#161) * fix: additional minor fixes after review (#163) * fix: BufferedMutator review fixes (#164) - Simplify #flush(). - Add javadocs. - (sequential) Fix flush() exception handling. - (sequential) Move error handling to a separate inner class. * fix: PR fixes * fix: report zeroed error metrics after successful operations * fix: prepend MismatchDetectorCounter with Test to better reflect its purpose * feat: Client-side timestamping (#165) * fix: reduce timeout in TestBlocking to make the tests run faster * fix: asyncClose -> closePrimaryAndScheduleSecondaryClose * fix: remove unused Batcher#throwBatchDataExceptionIfPresent * fix: remove unused Comparators#compareRows * fix: extract failedReads from MatchingSuccessfulReadsResults to reduce confusion * feat: remove unused MirroringTracer from FailedMutationLogger * fix: MirroringAsyncBufferedMutator - test if failed mutation is passed to secondary write error consumer * fix: TestMirroringAsyncTableInputModification typo fix * fix: describe user flush() in Buffered Mutator in quickstart * fix: MirroringBufferedMutator - move flush threshold from BufferedMutations to FlushSerializer * refactor: MirroringBufferedMutator#close() - use AccumulatedExceptions insteand of List<Exception> * BufferedMutator - add close timeout * AsyncBufferedMutator - add close timeout * fix: remove stale addSecondaryMutation comment * fix: add a comment that addSecondaryMutation handles failed writes * fix: unify implementations of flushBufferedMutatorBeforeClosing * fix: BufferedMutator - throw exceptions on close * fix: BufferedMutator - add comment explaining that chain of flush operations is created * fix: BufferedMutator - clarify comments * fix: Concurrent BufferedMutator - fix throwFlushExceptionIfAvailable * fix: explain why flush is called in Sequential BufferedMutator test * fix: TestConcurrentMirroringBufferedMutator - make waiting for calls explicit * refactor: BufferedMutator rename scheduleFlushAll() to scheduleFlush() * refactor: make FlushSerializer non-static * fix: BufferedMutator - use HierarchicalReferenceCounter * feat: Add MirroringConnection constructor taking MirroringConfiguration * refactor: move releaseReservations to finally * fix: use less convoluted example in lastFlushFutures description * fix: merge small Timeestamper files into a single file * fix: add a comment explaining which exceptions are forwarded to the user and why in SequentialMirroringBufferedMutator * fix: use UnsupportedOperationException instead of RuntimeException when forbidden mutation type is encountered * fix: add comment explaining why batch is complicated * fix: add a TODO to implement point writes without batch Co-authored-by: Mateusz Walkiewicz <mwalkiewicz@unoperate.com> Co-authored-by: Adam Czajkowski <prawilny@unoperate.com> Co-authored-by: Kajetan Boroszko <kajetan@unoperate.com>

mwalkiewicz force-pushed the mw/fix-buffered-mutator-close/1 branch from 074e0d5 to c876cf1 Compare October 22, 2021 11:32

mwalkiewicz force-pushed the mw/sync-in-2.x/1 branch from f2a2f5c to a2dd611 Compare October 22, 2021 11:34

mwalkiewicz linked an issue Oct 25, 2021 that may be closed by this pull request

2.x: synchronous (1.x) API #97

Closed

prawilny previously approved these changes Oct 27, 2021

View reviewed changes

mwalkiewicz commented Oct 29, 2021

View reviewed changes

mwalkiewicz force-pushed the mw/sync-in-2.x/1 branch from a2dd611 to 6aa1c94 Compare October 29, 2021 15:17

mwalkiewicz force-pushed the mw/fix-buffered-mutator-close/1 branch from c876cf1 to 9a3802b Compare November 5, 2021 15:40

feat: add synchronous MirroringConnection to 2.x

eb97797

mwalkiewicz force-pushed the mw/sync-in-2.x/1 branch from 6aa1c94 to eb97797 Compare November 9, 2021 13:21

mwalkiewicz changed the base branch from mw/fix-buffered-mutator-close/1 to master November 9, 2021 13:21

prawilny approved these changes Nov 9, 2021

View reviewed changes

prawilny reviewed Nov 9, 2021

View reviewed changes

prawilny self-requested a review November 9, 2021 14:12

prawilny reviewed Nov 9, 2021

View reviewed changes

mwalkiewicz commented Nov 9, 2021

View reviewed changes

mwalkiewicz merged commit 4977b40 into master Nov 9, 2021

dopiera pushed a commit that referenced this pull request Dec 2, 2021

feat: add synchronous MirroringConnection to 2.x (#109)

846969e

dopiera pushed a commit that referenced this pull request Dec 2, 2021

feat: add synchronous MirroringConnection to 2.x (#109)

91380e2

dopiera pushed a commit that referenced this pull request Dec 2, 2021

feat: add synchronous MirroringConnection to 2.x (#109)

be459b4

dopiera pushed a commit that referenced this pull request May 11, 2022

feat: add synchronous MirroringConnection to 2.x (#109)

b9b9b59

dopiera pushed a commit that referenced this pull request May 11, 2022

feat: add synchronous MirroringConnection to 2.x (#109)

58a2436

dopiera pushed a commit that referenced this pull request May 11, 2022

feat: add synchronous MirroringConnection to 2.x (#109)

2b4cd4f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add synchronous MirroringConnection to 2.x #109

feat: add synchronous MirroringConnection to 2.x #109

Uh oh!

mwalkiewicz commented Oct 22, 2021 •

edited by dopiera

Loading

Uh oh!

prawilny left a comment

Uh oh!

mwalkiewicz left a comment

Uh oh!

prawilny left a comment

Uh oh!

prawilny left a comment

Uh oh!

prawilny left a comment

Uh oh!

prawilny left a comment

Uh oh!

mwalkiewicz left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

feat: add synchronous MirroringConnection to 2.x #109

feat: add synchronous MirroringConnection to 2.x #109

Uh oh!

Conversation

mwalkiewicz commented Oct 22, 2021 • edited by dopiera Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prawilny left a comment

Choose a reason for hiding this comment

Uh oh!

mwalkiewicz left a comment

Choose a reason for hiding this comment

Uh oh!

prawilny left a comment

Choose a reason for hiding this comment

Uh oh!

prawilny left a comment

Choose a reason for hiding this comment

Uh oh!

prawilny left a comment

Choose a reason for hiding this comment

Uh oh!

prawilny left a comment

Choose a reason for hiding this comment

Uh oh!

mwalkiewicz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mwalkiewicz commented Oct 22, 2021 •

edited by dopiera

Loading