fix: force Append and Increment to return results and discard that result before returning it to user #136

prawilny · 2021-11-03T16:26:38Z

The name of the PR is WIP.
I only checked HBase 2.x behaviour for now.
We still probably need to implement similiar batch-append/increment result ignoring in #116
The tests suddenly refused to be run so I leave it as a draft for now.

This change is

prawilny · 2021-11-11T23:57:50Z

I didn't run the new integration tests yet.
Not implemended for AsyncMirroringBufferedMutator yet - will do it after a PR allowing us to create it from AsyncMirroringConnection.

prawilny · 2021-11-12T17:55:15Z

I realized these tests may as well be unit tests rather than integration tests. Rebased the PR onto master.

mwalkiewicz

Reviewable status: 0 of 6 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/utils/OperationUtils.java, line 84 at r1 (raw file):

  }

  public static class RewrittenOperations<T extends Row> {

RewrittenOperations -> RewrittenIncrementAndAppendIndicesInfo?

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

     *   <li>HBase 1.4.12
     *       <ul>
     *         <li>append(Append): null

I'd ignore this behavior and put Result.create(new Cell[0]) in batch operations, which will simplify this code.

prawilny

Reviewable status: 0 of 6 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/utils/OperationUtils.java, line 84 at r1 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

RewrittenOperations -> RewrittenIncrementAndAppendIndicesInfo?

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/OperationUtils.java, line 92 at r1 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

I'd ignore this behavior and put Result.create(new Cell[0]) in batch operations, which will simplify this code.

Done.

...lient-1.x/src/test/java/com/google/cloud/bigtable/mirroring/hbase1_x/TestMirroringTable.java

...ent-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/utils/OperationUtils.java

...lient-1.x/src/test/java/com/google/cloud/bigtable/mirroring/hbase1_x/TestMirroringTable.java

…sult before returning it to user (#136)

… 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>

prawilny force-pushed the ac/integration-tests-2.x/1 branch 2 times, most recently from 8fb010a to 1dac7e4 Compare November 10, 2021 14:10

prawilny force-pushed the ac/append-increment-force-results branch 2 times, most recently from 8ca87b9 to 08142e2 Compare November 11, 2021 23:47

prawilny requested a review from mwalkiewicz November 11, 2021 23:47

prawilny linked an issue Nov 11, 2021 that may be closed by this pull request

1.x/2.x: we should force increment to return results #123

Closed

prawilny force-pushed the ac/append-increment-force-results branch from 08142e2 to ecb8f1e Compare November 11, 2021 23:57

prawilny marked this pull request as ready for review November 11, 2021 23:57

prawilny force-pushed the ac/append-increment-force-results branch 2 times, most recently from 2c248f5 to e9f5eda Compare November 12, 2021 17:55

prawilny changed the base branch from ac/integration-tests-2.x/1 to master November 12, 2021 17:55

mwalkiewicz suggested changes Nov 23, 2021

View reviewed changes

prawilny commented Nov 23, 2021

View reviewed changes

prawilny force-pushed the ac/append-increment-force-results branch 2 times, most recently from 96bdb2b to 6a6b4c1 Compare November 23, 2021 15:06

mwalkiewicz suggested changes Nov 23, 2021

View reviewed changes

prawilny requested a review from mwalkiewicz November 23, 2021 16:12

mwalkiewicz reviewed Nov 24, 2021

View reviewed changes

...lient-1.x/src/test/java/com/google/cloud/bigtable/mirroring/hbase1_x/TestMirroringTable.java Outdated Show resolved Hide resolved

mwalkiewicz reviewed Nov 24, 2021

View reviewed changes

...lient-1.x/src/test/java/com/google/cloud/bigtable/mirroring/hbase1_x/TestMirroringTable.java Outdated Show resolved Hide resolved

mwalkiewicz reviewed Nov 24, 2021

View reviewed changes

...lient-1.x/src/test/java/com/google/cloud/bigtable/mirroring/hbase1_x/TestMirroringTable.java Outdated Show resolved Hide resolved

mwalkiewicz reviewed Nov 24, 2021

View reviewed changes

...lient-1.x/src/test/java/com/google/cloud/bigtable/mirroring/hbase1_x/TestMirroringTable.java Outdated Show resolved Hide resolved

prawilny force-pushed the ac/append-increment-force-results branch from c9d273f to b37a96c Compare November 24, 2021 18:11

prawilny requested a review from mwalkiewicz November 24, 2021 18:11

prawilny added 5 commits November 25, 2021 14:10

Force returning results by Increments and Appends

c1622e7

review fixes

0416794

review fixes 2

39f36c0

review fixes 3

ace226c

review fixes n+1

e6ac77d

mwalkiewicz force-pushed the ac/append-increment-force-results branch from 0772ece to e6ac77d Compare November 25, 2021 13:10

mwalkiewicz approved these changes Nov 25, 2021

View reviewed changes

mwalkiewicz merged commit 8488cd8 into master Nov 25, 2021

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

fix: force Append and Increment to return results and discard that re…

0b32edd

…sult before returning it to user (#136)

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

fix: force Append and Increment to return results and discard that re…

ed51996

…sult before returning it to user (#136)

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

fix: force Append and Increment to return results and discard that re…

f9a87f8

…sult before returning it to user (#136)

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

fix: force Append and Increment to return results and discard that re…

148c5c7

…sult before returning it to user (#136)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: force Append and Increment to return results and discard that result before returning it to user #136

fix: force Append and Increment to return results and discard that result before returning it to user #136

Uh oh!

prawilny commented Nov 3, 2021 •

edited by dopiera

Loading

Uh oh!

prawilny commented Nov 11, 2021

Uh oh!

prawilny commented Nov 12, 2021

Uh oh!

mwalkiewicz left a comment

Uh oh!

prawilny left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix: force Append and Increment to return results and discard that result before returning it to user #136

fix: force Append and Increment to return results and discard that result before returning it to user #136

Uh oh!

Conversation

prawilny commented Nov 3, 2021 • edited by dopiera Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prawilny commented Nov 11, 2021

Uh oh!

prawilny commented Nov 12, 2021

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

prawilny commented Nov 3, 2021 •

edited by dopiera

Loading