feat: synchronous writes #88

mwalkiewicz · 2021-10-19T09:50:11Z

This change is

dopiera

I think this looks really good in general, but I left some comments.

Reviewable status: 0 of 13 files reviewed, 11 unresolved discussions (waiting on @dopiera and @mwalkiewicz)

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

Instances

Instance in case of a class might be misleading - how about Objects?

bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/MirroringOperationException.java, line 31 at r1 (raw file):
How about slightly reformulating it?

These exceptions are attached as the root cause of the exception chain (as defined by getCause()) returned by the MirroringClient when it is set to synchronous mode. One can easily retrieve it by ...

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

Might be present only if operation failed on both databases.

I know that we are the only ones creating these objects, but perhaps adding an assertion on this invariant somewhere will prevent out future selves from shooting us in the foot?

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

lastException

rootCause, maybe?

Also, perhaps you can use Throwables.getRooCause() from guava?

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

extractMirroringOperationException

How about extractFromRootCause()?

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

  }

  private static final String unknownHostnamePort = "";

Shouldn't this default be in the ctor?

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

          getExceptionDetails(failedSecondaryOperations, secondaryOperation);

      Throwable exception;

You could use MirroringOperationException instead of Throwable, no?

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

      boolean primaryIsRead = primaryBatchData.operations.get(primaryIndex) instanceof Get;
      boolean secondaryIsRead = secondaryBatchData.operations.get(secondaryIndex) instanceof Get;
      if (primaryIsRead && !secondaryIsRead) {

If you've got two consecutive reads and one of them is excluded, how do you know which one?

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

      // Otherwise a successful write was excluded, which is not possible.
      assert primaryIsRead == secondaryIsRead;

primaryIsRead == secondaryIsRead == false

right?

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

  public static Map<Row, ExceptionDetails> makeMapOfFailedRows(BatchData primaryBatchData) {
    HashMap<Row, ExceptionDetails> result = new HashMap<>();

Isn't this a case for IdentityHashMap?

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

   * reflect errors that happened on one of the databases. Types of thrown exceptions are not
   * changed, but a {@link com.google.cloud.bigtable.mirroring.hbase1_x.MirroringOperationException}
   * is added as a cause for thrown exceptions (For more details see {@link

s/cause/root cause/g

mwalkiewicz

Reviewable status: 0 of 13 files reviewed, 11 unresolved discussions (waiting on @dopiera)

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

Previously, dopiera (Marek Dopiera) wrote…

Instances
Instance in case of a class might be misleading - how about Objects?

Done.

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

Previously, dopiera (Marek Dopiera) wrote…

How about slightly reformulating it?

These exceptions are attached as the root cause of the exception chain (as defined by getCause()) returned by the MirroringClient when it is set to synchronous mode. One can easily retrieve it by ...

Done.

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

Previously, dopiera (Marek Dopiera) wrote…

Might be present only if operation failed on both databases.
I know that we are the only ones creating these objects, but perhaps adding an assertion on this invariant somewhere will prevent out future selves from shooting us in the foot?

Done.

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

Previously, dopiera (Marek Dopiera) wrote…

lastException
rootCause, maybe?

Also, perhaps you can use Throwables.getRooCause() from guava?

Done.

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

Previously, dopiera (Marek Dopiera) wrote…

extractMirroringOperationException
How about extractFromRootCause()?

Done.

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

Previously, dopiera (Marek Dopiera) wrote…

Shouldn't this default be in the ctor?

Done.

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

Previously, dopiera (Marek Dopiera) wrote…

You could use MirroringOperationException instead of Throwable, no?

No, markedAsPrimaryException returns exception of the same kind as its input (in fact, it returns the same exception now), and primaryExceptionDetails.exception is Throwable.

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

Previously, dopiera (Marek Dopiera) wrote…

If you've got two consecutive reads and one of them is excluded, how do you know which one?

I assume that this won't happen. We only exclude reads from primary if reads are not sampled.

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

Previously, dopiera (Marek Dopiera) wrote…

primaryIsRead == secondaryIsRead == false

right?

No, sequential batch can contain reads. They do not have to be excluded.

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

Previously, dopiera (Marek Dopiera) wrote…

Isn't this a case for IdentityHashMap?

Done. You are right, I should also change other maps that hold Rows to this class. I'll create an issue.

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

Previously, dopiera (Marek Dopiera) wrote…

s/cause/root cause/g

Done.

dopiera

Reviewable status: 0 of 13 files reviewed, 2 unresolved discussions (waiting on @dopiera and @mwalkiewicz)

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

      DatabaseIdentifier databaseIdentifier, Row operation, ExceptionDetails secondaryException) {
    Preconditions.checkArgument(
        secondaryException == null || databaseIdentifier == DatabaseIdentifier.Both);

This is not "if and only if". You can have secondary exception set to null and DB identifier set to both.

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

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

I assume that this won't happen. We only exclude reads from primary if reads are not sampled.

Oh - right I forgot that we went with an all or nothing approach in a batch. Could you please leave a comment pointing out this assumption for posterity?

mwalkiewicz

Reviewable status: 0 of 13 files reviewed, 2 unresolved discussions (waiting on @dopiera)

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

Previously, dopiera (Marek Dopiera) wrote…

This is not "if and only if". You can have secondary exception set to null and DB identifier set to both.

This is intended, secondary error can be null when flowController rejects the reservation request. Then we have to report an error that prevented a write on both databases, but we do not have a secondary error to attach. I'll rephrase secondaryException's doc.

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

Previously, dopiera (Marek Dopiera) wrote…

Oh - right I forgot that we went with an all or nothing approach in a batch. Could you please leave a comment pointing out this assumption for posterity?

Done.

dopiera

Reviewed 7 of 13 files at r1, 2 of 4 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @mwalkiewicz)

The base branch was changed.

dopiera

Reviewed 6 of 6 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @mwalkiewicz)

… 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/sync-writes/1 branch from 7bfb6b4 to 16d5608 Compare October 19, 2021 10:08

dopiera requested changes Oct 25, 2021

View reviewed changes

mwalkiewicz force-pushed the mw/sync-writes/1 branch from 16d5608 to 8f337f6 Compare October 26, 2021 09:42

mwalkiewicz commented Oct 26, 2021

View reviewed changes

dopiera requested changes Oct 26, 2021

View reviewed changes

mwalkiewicz force-pushed the mw/sync-writes/1 branch from 8f337f6 to e30d6e7 Compare October 26, 2021 10:37

mwalkiewicz commented Oct 26, 2021

View reviewed changes

mwalkiewicz force-pushed the mw/concurrent-writes-buffered-mutator/1 branch from 4896f31 to 0320584 Compare October 26, 2021 10:42

mwalkiewicz force-pushed the mw/sync-writes/1 branch 2 times, most recently from 4a01aa5 to d44c5f9 Compare October 26, 2021 10:45

mwalkiewicz changed the base branch from mw/concurrent-writes-buffered-mutator/1 to mw/request-scheduling-refactor/1 October 26, 2021 10:46

dopiera previously approved these changes Oct 26, 2021

View reviewed changes

mwalkiewicz force-pushed the mw/sync-writes/1 branch from d44c5f9 to 3f094ae Compare November 5, 2021 08:58

feat: synchronous writes

738ea8b

mwalkiewicz force-pushed the mw/sync-writes/1 branch from 3f094ae to 738ea8b Compare November 5, 2021 08:59

mwalkiewicz changed the base branch from mw/request-scheduling-refactor/1 to master November 5, 2021 08:59

mwalkiewicz requested a review from dopiera November 5, 2021 09:01

dopiera approved these changes Nov 5, 2021

View reviewed changes

mwalkiewicz merged commit 4f2040d into master Nov 5, 2021

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

feat: synchronous writes (#88)

83c066f

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

feat: synchronous writes (#88)

abb68c5

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

feat: synchronous writes (#88)

f9aca07

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

feat: synchronous writes (#88)

182bf3d

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

feat: synchronous writes (#88)

282a361

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: synchronous writes #88

feat: synchronous writes #88

Uh oh!

mwalkiewicz commented Oct 19, 2021 •

edited by dopiera

Loading

Uh oh!

dopiera left a comment

Uh oh!

mwalkiewicz left a comment

Uh oh!

dopiera left a comment

Uh oh!

mwalkiewicz left a comment

Uh oh!

dopiera left a comment

Uh oh!

dopiera left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

feat: synchronous writes #88

feat: synchronous writes #88

Uh oh!

Conversation

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

Uh oh!

Uh oh!

dopiera 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!

dopiera 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!

dopiera left a comment

Choose a reason for hiding this comment

Uh oh!

dopiera 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 19, 2021 •

edited by dopiera

Loading