Skip to content

Conversation

@prawilny
Copy link
Collaborator

@prawilny prawilny commented Sep 29, 2021

This change is Reviewable

Copy link

@mwalkiewicz mwalkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @dopiera 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 134 at r1 (raw file):

  }

  public static Put makePutFromResult(Result result) {

If you want to reuse this method move it to a separate package/class and use it in both places. Helper method do not belong to a public API of a class, unless it is a class with helpers.


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

    CompletableFuture<Result> primaryFuture = this.primaryTable.append(append);
    primaryFuture.whenComplete(

Instead of copying the logic twice extract it to a separate method. Maybe you can integrate it somehow with


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

              .whenComplete(
                  (ignoredResult, ignoredError) -> {
                    resultFuture.complete(primaryResult);

The resultFuture is completed after both operation are completed, not after reservation is taken. Please do not reinvent logic that is already implemented, try to make it generic and reuse it.

Copy link

@mwalkiewicz mwalkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @dopiera and @prawilny)


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

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

Instead of copying the logic twice extract it to a separate method. Maybe you can integrate it somehow with

... with writeWithFlowControl, e.g. by using just a little more generics.

Copy link
Collaborator Author

@prawilny prawilny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

... with writeWithFlowControl, e.g. by using just a little more generics.

Done.


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

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

The resultFuture is completed after both operation are completed, not after reservation is taken. Please do not reinvent logic that is already implemented, try to make it generic and reuse it.

Done.

Copy link

@dopiera dopiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 3 files reviewed, 3 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/MirroringTable.java, line 134 at r1 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

If you want to reuse this method move it to a separate package/class and use it in both places. Helper method do not belong to a public API of a class, unless it is a class with helpers.

+1

@prawilny prawilny force-pushed the ac/increment-append-as-put branch from acaf88c to f49f470 Compare September 30, 2021 15:22
Copy link
Collaborator Author

@prawilny prawilny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissed @mwalkiewicz from a discussion.
Reviewable status: 0 of 4 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/MirroringTable.java, line 134 at r1 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

+1

Done. I forgot about it last time.

dopiera
dopiera previously approved these changes Sep 30, 2021
Copy link

@dopiera dopiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@mwalkiewicz mwalkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @prawilny)


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

        HBaseOperation.INCREMENT,
        primaryTable.increment(increment),
        () -> this.secondaryWriteErrorConsumer.consume(HBaseOperation.INCREMENT, increment));

We should use converted Put here. It'd be the best to move this handler to mutationViaPut.

@prawilny prawilny force-pushed the ac/increment-append-as-put branch from f49f470 to 307d972 Compare October 1, 2021 14:11
@prawilny prawilny changed the base branch from master to ac/remove-redundant-writeWithControlFlow-arugment October 1, 2021 14:15
Copy link
Collaborator Author

@prawilny prawilny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

We should use converted Put here. It'd be the best to move this handler to mutationViaPut.

Done.


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

                  () -> this.secondaryTable.put(put),
                  () -> secondaryPutWriteErrorHandler.accept(put))
              .whenComplete(

Fixed a bug with too early completion of the result future. As I wrote somewhere else, I hope at some point I'll find time to create some shared functions to reduce such boilerplate.

@prawilny prawilny force-pushed the ac/increment-append-as-put branch from 307d972 to 8a66cfb Compare October 1, 2021 15:02
Copy link

@mwalkiewicz mwalkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dopiera and @prawilny)


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

Previously, prawilny (Adam Czajkowski) wrote…

Done.

You should pass HBaseOperation.PUT here and in append. Then you can move creating this handler mutationAsPut.


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

Previously, prawilny (Adam Czajkowski) wrote…

Fixed a bug with too early completion of the result future. As I wrote somewhere else, I hope at some point I'll find time to create some shared functions to reduce such boilerplate.

You can surely do this when you finish currently opened tasks. I do not think that we are in a rush.


bigtable-hbase-mirroring-client-2.x-parent/bigtable-hbase-mirroring-client-2.x/src/test/java/com/google/cloud/bigtable/mirroring/hbase2_x/TestMirroringAsyncTable.java, line 591 at r5 (raw file):

  }

  private void assertPutsAreEqual(Put expectedPut, Put value) {

Can we add a dependency to 1.x tests and use method defined there? I think it is enough of temporary copy-pastes and it's time to invent some sane solution to this problem.


bigtable-hbase-mirroring-client-2.x-parent/bigtable-hbase-mirroring-client-2.x/src/test/java/com/google/cloud/bigtable/mirroring/hbase2_x/TestMirroringAsyncTable.java, line 635 at r5 (raw file):

    ArgumentCaptor<Put> argument = ArgumentCaptor.forClass(Put.class);
    verify(secondaryTable, never()).increment(any(Increment.class));
    verify(secondaryTable, times(3)).put(argument.capture());

You can just use .put(expectedPut) and times(3) will handle checking if it was called 3 times, no need to use argument captor. If you want to be sure that is was not called with any other argument, you can add a check verify(..., times(3)).put(any()).


bigtable-hbase-mirroring-client-2.x-parent/bigtable-hbase-mirroring-client-2.x/src/test/java/com/google/cloud/bigtable/mirroring/hbase2_x/TestMirroringAsyncTable.java, line 667 at r5 (raw file):

    ArgumentCaptor<Put> argument = ArgumentCaptor.forClass(Put.class);
    verify(secondaryTable, times(1)).put(argument.capture());

There's no need to use ArgumentCaptor.

dopiera
dopiera previously approved these changes Oct 4, 2021
Copy link

@dopiera dopiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @prawilny)

@prawilny prawilny linked an issue Oct 5, 2021 that may be closed by this pull request
@prawilny prawilny force-pushed the ac/remove-redundant-writeWithControlFlow-arugment branch from 4e554c1 to e408214 Compare October 5, 2021 16:42
Copy link
Collaborator Author

@prawilny prawilny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

You should pass HBaseOperation.PUT here and in append. Then you can move creating this handler mutationAsPut.

Done.


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

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

You can surely do this when you finish currently opened tasks. I do not think that we are in a rush.

Opened the pull request with FutureUtils.


bigtable-hbase-mirroring-client-2.x-parent/bigtable-hbase-mirroring-client-2.x/src/test/java/com/google/cloud/bigtable/mirroring/hbase2_x/TestMirroringAsyncTable.java, line 591 at r5 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

Can we add a dependency to 1.x tests and use method defined there? I think it is enough of temporary copy-pastes and it's time to invent some sane solution to this problem.

No, there's a problem: CellComparator in HBase1 is a class and in HBase2 - an interface, so even brutally casting it doesn't work.


bigtable-hbase-mirroring-client-2.x-parent/bigtable-hbase-mirroring-client-2.x/src/test/java/com/google/cloud/bigtable/mirroring/hbase2_x/TestMirroringAsyncTable.java, line 635 at r5 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

You can just use .put(expectedPut) and times(3) will handle checking if it was called 3 times, no need to use argument captor. If you want to be sure that is was not called with any other argument, you can add a check verify(..., times(3)).put(any()).

Unfortunately, there comes up a problem I couldn't google:
image.png
How should I deal with it properly?
I feel that lowering the tests' strictness (any()) isn't the solution.


bigtable-hbase-mirroring-client-2.x-parent/bigtable-hbase-mirroring-client-2.x/src/test/java/com/google/cloud/bigtable/mirroring/hbase2_x/TestMirroringAsyncTable.java, line 667 at r5 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

There's no need to use ArgumentCaptor.

Unfortunately, there is (sameish as above)
image.png

@prawilny prawilny force-pushed the ac/increment-append-as-put branch from 8a66cfb to edc6993 Compare October 5, 2021 17:14
@prawilny prawilny requested a review from mwalkiewicz October 5, 2021 18:06
Copy link

@mwalkiewicz mwalkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @prawilny)


bigtable-hbase-mirroring-client-2.x-parent/bigtable-hbase-mirroring-client-2.x/src/test/java/com/google/cloud/bigtable/mirroring/hbase2_x/TestMirroringAsyncTable.java, line 591 at r5 (raw file):

Previously, prawilny (Adam Czajkowski) wrote…

No, there's a problem: CellComparator in HBase1 is a class and in HBase2 - an interface, so even brutally casting it doesn't work.

OK, noted. But you can create a shared assertPutsAreEqual(Put, Put, CellComparator) in common package and then use it here and in 1.x tests.


bigtable-hbase-mirroring-client-2.x-parent/bigtable-hbase-mirroring-client-2.x/src/test/java/com/google/cloud/bigtable/mirroring/hbase2_x/TestMirroringAsyncTable.java, line 635 at r5 (raw file):

Previously, prawilny (Adam Czajkowski) wrote…

Unfortunately, there comes up a problem I couldn't google:
image.png
How should I deal with it properly?
I feel that lowering the tests' strictness (any()) isn't the solution.

Oh, you are right, assertPutsAreEqual won't be used by verify :/ Sorry for that.

@prawilny prawilny force-pushed the ac/increment-append-as-put branch from edc6993 to 5ec6637 Compare October 6, 2021 12:29
Copy link
Collaborator Author

@prawilny prawilny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @dopiera and @mwalkiewicz)


bigtable-hbase-mirroring-client-2.x-parent/bigtable-hbase-mirroring-client-2.x/src/test/java/com/google/cloud/bigtable/mirroring/hbase2_x/TestMirroringAsyncTable.java, line 591 at r5 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

OK, noted. But you can create a shared assertPutsAreEqual(Put, Put, CellComparator) in common package and then use it here and in 1.x tests.

Done.

@prawilny prawilny requested a review from mwalkiewicz October 6, 2021 12:34
@prawilny prawilny changed the base branch from ac/remove-redundant-writeWithControlFlow-arugment to master October 6, 2021 14:38
@prawilny prawilny dismissed dopiera’s stale review October 6, 2021 14:38

The base branch was changed.

@prawilny prawilny force-pushed the ac/increment-append-as-put branch from 5ec6637 to 42f827b Compare October 12, 2021 17:39
@prawilny prawilny changed the base branch from master to ac/Builders/2 October 12, 2021 17:39
Copy link
Collaborator Author

@prawilny prawilny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 6 files reviewed, 1 unresolved discussion (waiting on @dopiera and @mwalkiewicz)


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

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

Bug I've just noticed in my own code, and that also applies here:
the WriteOperationInfo should be created with Put that will be sent to the secondary database, not with Append sent to the primary.

Done.

@prawilny prawilny force-pushed the ac/increment-append-as-put branch from 42f827b to 0226ead Compare October 13, 2021 09:53
Copy link
Collaborator Author

@prawilny prawilny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissed @mwalkiewicz from a discussion.
Reviewable status: 1 of 6 files reviewed, 1 unresolved discussion (waiting on @dopiera and @mwalkiewicz)


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

          }
          Put put = makePutFromResult(primaryResult);
          FutureUtils.forwardResult(

Changed to approach from the CheckAndMutateBuilder PR.

Copy link

@dopiera dopiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 6 files reviewed, 1 unresolved discussion (waiting on @dopiera, @mwalkiewicz, and @prawilny)


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

Previously, prawilny (Adam Czajkowski) wrote…

Changed to approach from the CheckAndMutateBuilder PR.

Use exceptionally?

@prawilny prawilny force-pushed the ac/increment-append-as-put branch from 0226ead to 0c041e0 Compare October 13, 2021 17:02
Copy link
Collaborator Author

@prawilny prawilny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 6 files reviewed, 1 unresolved discussion (waiting on @dopiera and @mwalkiewicz)


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

Previously, dopiera (Marek Dopiera) wrote…

Use exceptionally?

As stated elsewhere (checkAndMutate PR, I think): I planned to make one big PR with style change across the codebase, but rewritten as per request.
Done.

@prawilny prawilny requested a review from dopiera October 13, 2021 17:03
dopiera
dopiera previously approved these changes Oct 14, 2021
Copy link

@dopiera dopiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 6 files reviewed, 1 unresolved discussion (waiting on @dopiera, @mwalkiewicz, and @prawilny)

@prawilny prawilny force-pushed the ac/increment-append-as-put branch from 0c041e0 to 66cd942 Compare October 15, 2021 15:29
@prawilny prawilny force-pushed the ac/Builders/2 branch 2 times, most recently from 9f1176f to 207cc1d Compare October 15, 2021 15:47
@prawilny prawilny force-pushed the ac/increment-append-as-put branch from 66cd942 to d401cce Compare October 15, 2021 15:55
@prawilny prawilny changed the base branch from ac/Builders/2 to master October 15, 2021 15:55
@prawilny prawilny dismissed dopiera’s stale review October 15, 2021 15:55

The base branch was changed.

@prawilny prawilny force-pushed the ac/increment-append-as-put branch from d401cce to 521a9d3 Compare October 15, 2021 16:10
Copy link

@mwalkiewicz mwalkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 5 files at r9, 3 of 3 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @prawilny)

@prawilny prawilny merged commit 18d60ec into master Oct 15, 2021
@prawilny prawilny deleted the ac/increment-append-as-put branch November 2, 2021 14:00
dopiera added a commit that referenced this pull request May 11, 2022
* Initial implementation of hbase1.x Mirroring Client (#1)

* Initial implementation of hbase1.x Mirroring Client

* MirroringTable and AsyncTableWrapper (#4)

* MirroringTable and AsyncTableWrapper

- Initial implementation of MirroringTable.
- Wrapper around HBase 1.x Table enabling scheduling asynchronous operations.
- Asynchronous verification of get/exists results.

* Add missing licence header to TestResultComparator (#6)

* Setup running tests in GitHub Actions (#7)

* Asynchronous ResultScanner (#5)

* Asynchronous ResultScanner

* Implement faillog.Appender (#2)

`faillog.Appender` is the lowermost component of the infrastructure for
dumping failed mutations in the `MirroringClient`. `faillog/README.md`
explains the design decisions in a bit more detail.

`faillog.Appender` essentially logs arbitrary data asynchronously,
through a separate thread reading from a bounded buffer.

* ListenableReferenceCounter (#8)

* Count references in asynchronous tasks before closing Table/Scanner.

* Flow controller (#10)

* Flow Controller
* Flow Control strategy based on single requests queue

* Mirroring table: writes (#12)

* Add missing condition in result comparator (#15)

* Add more mirroring config opitons (#16)

* MismatchDetector implementation.
* FlowControllerStrategy implementation.
* Maximal number of outstanding requests used by FlowControllerStrategy.
* Primary/Secondary connection implementation option now accepts
  "default" value which can be used when default HBase Connection
  implementation should be used by MirroringConnection.

* Make AsyncTableWrapper ListenableCloseable (#20)

Implements our standard interface for objects that can run callbacks
after asynchronous close to simplify reference counting of
MirroringTable.

* MirroringConnection: use new config options (#17)

* MirroringResultScanner improvements (#18)

* Count references to MirroringResultScanner

Current implementation of MirroringResultScanner doesn't count
verificaiton requests that it has scheduled and allows to close
instances while verificaiton requests are in-flight. This causes lost
verifications.

This PR fixes this issue by counting references to MirroringResultScanner
instances when scheduling verification requests. Moreover, ListenableClosable
interface is implemented for consistency with other classes that use this
scheme, because now the MirroringResultScanner instances will be closed
asynchronously, when all scheduled requests have finished.

* MirroringTable: count references to scanners and secondary wrapper (#21)

Current MirroringTable implementaion does not count its references held
by MirroringResultScanners and SecondaryAsyncWrapper, thus
MirroringConnection consideres it closed before we are sure that all
asynchronous operations have completed.

This PR adds correct reference counting of MirroringTable based on work
done in previously merged PRs.

* Result Scanner - ensure verification ordering (#22)

Current implementation assumes that next() operations on primary and
secondary scanners are called in the same order and uses this assumption
to match results for verification.

However, next()s on secondary database are called asynchronusly and
their order is not defined, which causes invalid mismatch reports.

This PR fixes this problem by placing data to be verified - results of
next()s called on primary scanner - and details of next() call - number
of requested elements - call on a queue. Each asynchronous call to
next() is synchonized and pops a single element from that queue.
Appropriate next is called based on number of requested elements.
Then results of that request and results from the queue are verified.
This ensures that results of next()s passed to verification are
correctly matched and ordered.

* Integration tests (#23)

HBase 1.x integration tests

* Add trace logging. (#24)

* Estimate memory overhead in RequestResourceDescription (#25)

* Tests: extract executor service to TestRule (#26)

Extract executor service utilities into TestRule to facilitate code
reuse in other test classes.

* Integration tests: read configuration from xml files

* MirroringBufferedMutator

* MirroringBufferedMutator: integration tests (#9)

* Fix error introduced in rebase (#11)

* Obtain resource reservation before scheduling secondary calls (#4)

Fixes a bug when secondary database request Future was created before obtaining resources from FlowController.

* Integration tests - MirroringTable operations (#10)

* MirroringAsyncConfiguration (#5)

Add configuration class to be used by MirroringAsyncConnection.

* SecondaryWriteErrorConsumer in MirroringTable (#15)

Use SecondaryWriteErrorConsumer to handle write errors in secondary database in MirroringTable's writes.

* Use Put to implement Increment and Append (#16)

* refactor: extract functions using reflection into package utils.reflection

* refactor: extract BatchHelpers into utils

Extract common part of batch() helpers into a class and add Predicate argument to nested classes' constructors making it possible to reuse the code in 2.x client.

* feat: Initial implementation of a subset of asynchronous HBase 2.x Mirroring Client

Contains basic implementation of MirroringAsyncConnection and MirroringAsyncTable.

* refactor: extract FlowController's request cancellation into a method

* fix: Increment ITs fail with Bigtable as primary (#21)

We were setting timerange on Increment objects used in integration tests
without any reason and Bigtable doesn't support this operation. Setting
timerange in ITs was removed.

* fix: RequestScheduling should handle rejected resource reservations (#24)

Custom FlowControlerStrategy implementations might, contrary to the
default implementation, resolve reservation requests with exception,
what we should handle by not performing the action that had to acquire
the resources.

* feat: Add OpenCensus tracing and metrics. (#14)

* fix: make BatchHelpers skip verification of empty read results

BatchHelpers provides error handling of batch() when there may be some partial results.
Before the commit, matching successful reads were redundantly verified if there were none of them.
This commit brings back the behaviour from up to 5a29253: when there are no successful matching reads, a MismatchDetector isn't called on empty arrays.

* refactor: make MirroringAsync{Connection,Table} use SecondaryWriteErrorConsumerWithMetrics

BatchHelpers require using SecondaryWriteErrorConsumerWithMetrics API.

* refactor: make AsyncRequestScheduling accept CompletableFuture<ResourceReservation> instead of ResourceReservation

This change is split off from commit introducing MirroringAsyncTable#batch()

* feat: implement batch() in MirroringAsyncTable

Implementation of MirroringAsyncTable's batch() and
MirroringAsyncTable's methods such as get(List<Get) and put(List<Put>) using it.

* feat: implement failed mutations log (#19)

Failed secondary mutations are written to disk in JSON format, which the user can parse programmatically or inspect visually.

Each failure is logged as a separate line, which makes it compatible with solutions like logstash.

* refactor: split SplitBatchResponse (#40)

SplitBatchResponse was refactored into two parts: splitting into
reads/writes and failed/successful. This makes the code simpler and
easier to maintain.

* refactor: extract helper methods in tests (#48)

* refactor: remove redundant writeWithControlFlow argument

* feat: copy HBase operations' input lists (#57)

* refactor: remove redundant field from MirroringConnection (#55)

* feat: verification with sampling (#28)

* fix: mirror Increment/Append in batch() using Put. (#47)

* refactor: Move HBaseOperation into WriteOperationInfo (#68)

* refactor: remove redundant parameter from scheduleWriteWithControlFlow (#69)

* fix: integration tests - fix build (#70)

* fix: count references to batch operations (#63)

* fix: close underlying connections when MirroringConnection is closed (#49)

* refactor: fix IDE warnings in MirroringAsyncTable test (#64)

* fix: integration tests - check if write errors were reported (#71)

* feat: make SecondaryWriteErrorConsumer accept error cause and operation (#65)

* fix: do not call callbacks with lock held (#53)

* refactor: use AccumulatedExceptions where appropriate (#54)

* fix: fix key used in verification sampling ITs (#77)

* feat: use faillog for handling write errors (#66)

* refactor: add utilities for Futures (FutureUtils)

* feat: defer closing connections until async operations complete (#37)

Mirroring client schedules asynchronous operations - to mirror the mutations and to verify reads. Before this PR, closing the MirroringAsyncConnection would result in closing the underlying connections immediately. This made the pending asynchronous operations fail. This PR defers closing the underlying connections until all pending operations complete. It is achieved by reference counting the operations.

* feat: implement AsyncTableBuilder (#42)

* feat: implement MirroringAsyncTable#checkAndMutate (#43)

* fix: Implement single Append and Increment as Put (#38)

* refactor: simplify SecondaryWriteErrorConsumer API (#78)

* feat: concurrent writes in MirroringTable (#79)

* test: fix failing concurrent write test (#120)

* refactor: renames and moves in RequestScheduling (#87)

* wip: handover session comments

Co-authored-by: Mateusz Walkiewicz <mwalkiewicz@unoperate.com>
Co-authored-by: Adam Czajkowski <prawilny@unoperate.com>
mwalkiewicz added a commit that referenced this pull request May 18, 2022
* Initial implementation of hbase1.x Mirroring Client (#1)

* Initial implementation of hbase1.x Mirroring Client

* MirroringTable and AsyncTableWrapper (#4)

* MirroringTable and AsyncTableWrapper

- Initial implementation of MirroringTable.
- Wrapper around HBase 1.x Table enabling scheduling asynchronous operations.
- Asynchronous verification of get/exists results.

* Add missing licence header to TestResultComparator (#6)

* Setup running tests in GitHub Actions (#7)

* Asynchronous ResultScanner (#5)

* Asynchronous ResultScanner

* Implement faillog.Appender (#2)

`faillog.Appender` is the lowermost component of the infrastructure for
dumping failed mutations in the `MirroringClient`. `faillog/README.md`
explains the design decisions in a bit more detail.

`faillog.Appender` essentially logs arbitrary data asynchronously,
through a separate thread reading from a bounded buffer.

* ListenableReferenceCounter (#8)

* Count references in asynchronous tasks before closing Table/Scanner.

* Flow controller (#10)

* Flow Controller
* Flow Control strategy based on single requests queue

* Mirroring table: writes (#12)

* Add missing condition in result comparator (#15)

* Add more mirroring config opitons (#16)

* MismatchDetector implementation.
* FlowControllerStrategy implementation.
* Maximal number of outstanding requests used by FlowControllerStrategy.
* Primary/Secondary connection implementation option now accepts
  "default" value which can be used when default HBase Connection
  implementation should be used by MirroringConnection.

* Make AsyncTableWrapper ListenableCloseable (#20)

Implements our standard interface for objects that can run callbacks
after asynchronous close to simplify reference counting of
MirroringTable.

* MirroringConnection: use new config options (#17)

* MirroringResultScanner improvements (#18)

* Count references to MirroringResultScanner

Current implementation of MirroringResultScanner doesn't count
verificaiton requests that it has scheduled and allows to close
instances while verificaiton requests are in-flight. This causes lost
verifications.

This PR fixes this issue by counting references to MirroringResultScanner
instances when scheduling verification requests. Moreover, ListenableClosable
interface is implemented for consistency with other classes that use this
scheme, because now the MirroringResultScanner instances will be closed
asynchronously, when all scheduled requests have finished.

* MirroringTable: count references to scanners and secondary wrapper (#21)

Current MirroringTable implementaion does not count its references held
by MirroringResultScanners and SecondaryAsyncWrapper, thus
MirroringConnection consideres it closed before we are sure that all
asynchronous operations have completed.

This PR adds correct reference counting of MirroringTable based on work
done in previously merged PRs.

* Result Scanner - ensure verification ordering (#22)

Current implementation assumes that next() operations on primary and
secondary scanners are called in the same order and uses this assumption
to match results for verification.

However, next()s on secondary database are called asynchronusly and
their order is not defined, which causes invalid mismatch reports.

This PR fixes this problem by placing data to be verified - results of
next()s called on primary scanner - and details of next() call - number
of requested elements - call on a queue. Each asynchronous call to
next() is synchonized and pops a single element from that queue.
Appropriate next is called based on number of requested elements.
Then results of that request and results from the queue are verified.
This ensures that results of next()s passed to verification are
correctly matched and ordered.

* Integration tests (#23)

HBase 1.x integration tests

* Add trace logging. (#24)

* Estimate memory overhead in RequestResourceDescription (#25)

* Tests: extract executor service to TestRule (#26)

Extract executor service utilities into TestRule to facilitate code
reuse in other test classes.

* Integration tests: read configuration from xml files

* MirroringBufferedMutator

* MirroringBufferedMutator: integration tests (#9)

* Fix error introduced in rebase (#11)

* Obtain resource reservation before scheduling secondary calls (#4)

Fixes a bug when secondary database request Future was created before obtaining resources from FlowController.

* Integration tests - MirroringTable operations (#10)

* MirroringAsyncConfiguration (#5)

Add configuration class to be used by MirroringAsyncConnection.

* SecondaryWriteErrorConsumer in MirroringTable (#15)

Use SecondaryWriteErrorConsumer to handle write errors in secondary database in MirroringTable's writes.

* Use Put to implement Increment and Append (#16)

* refactor: extract functions using reflection into package utils.reflection

* refactor: extract BatchHelpers into utils

Extract common part of batch() helpers into a class and add Predicate argument to nested classes' constructors making it possible to reuse the code in 2.x client.

* feat: Initial implementation of a subset of asynchronous HBase 2.x Mirroring Client

Contains basic implementation of MirroringAsyncConnection and MirroringAsyncTable.

* refactor: extract FlowController's request cancellation into a method

* fix: Increment ITs fail with Bigtable as primary (#21)

We were setting timerange on Increment objects used in integration tests
without any reason and Bigtable doesn't support this operation. Setting
timerange in ITs was removed.

* fix: RequestScheduling should handle rejected resource reservations (#24)

Custom FlowControlerStrategy implementations might, contrary to the
default implementation, resolve reservation requests with exception,
what we should handle by not performing the action that had to acquire
the resources.

* feat: Add OpenCensus tracing and metrics. (#14)

* fix: make BatchHelpers skip verification of empty read results

BatchHelpers provides error handling of batch() when there may be some partial results.
Before the commit, matching successful reads were redundantly verified if there were none of them.
This commit brings back the behaviour from up to 5a29253: when there are no successful matching reads, a MismatchDetector isn't called on empty arrays.

* refactor: make MirroringAsync{Connection,Table} use SecondaryWriteErrorConsumerWithMetrics

BatchHelpers require using SecondaryWriteErrorConsumerWithMetrics API.

* refactor: make AsyncRequestScheduling accept CompletableFuture<ResourceReservation> instead of ResourceReservation

This change is split off from commit introducing MirroringAsyncTable#batch()

* feat: implement batch() in MirroringAsyncTable

Implementation of MirroringAsyncTable's batch() and
MirroringAsyncTable's methods such as get(List<Get) and put(List<Put>) using it.

* feat: implement failed mutations log (#19)

Failed secondary mutations are written to disk in JSON format, which the user can parse programmatically or inspect visually.

Each failure is logged as a separate line, which makes it compatible with solutions like logstash.

* refactor: split SplitBatchResponse (#40)

SplitBatchResponse was refactored into two parts: splitting into
reads/writes and failed/successful. This makes the code simpler and
easier to maintain.

* refactor: extract helper methods in tests (#48)

* refactor: remove redundant writeWithControlFlow argument

* feat: copy HBase operations' input lists (#57)

* refactor: remove redundant field from MirroringConnection (#55)

* feat: verification with sampling (#28)

* fix: mirror Increment/Append in batch() using Put. (#47)

* refactor: Move HBaseOperation into WriteOperationInfo (#68)

* refactor: remove redundant parameter from scheduleWriteWithControlFlow (#69)

* fix: integration tests - fix build (#70)

* fix: count references to batch operations (#63)

* fix: close underlying connections when MirroringConnection is closed (#49)

* refactor: fix IDE warnings in MirroringAsyncTable test (#64)

* fix: integration tests - check if write errors were reported (#71)

* feat: make SecondaryWriteErrorConsumer accept error cause and operation (#65)

* fix: do not call callbacks with lock held (#53)

* refactor: use AccumulatedExceptions where appropriate (#54)

* fix: fix key used in verification sampling ITs (#77)

* feat: use faillog for handling write errors (#66)

* refactor: add utilities for Futures (FutureUtils)

* feat: defer closing connections until async operations complete (#37)

Mirroring client schedules asynchronous operations - to mirror the mutations and to verify reads. Before this PR, closing the MirroringAsyncConnection would result in closing the underlying connections immediately. This made the pending asynchronous operations fail. This PR defers closing the underlying connections until all pending operations complete. It is achieved by reference counting the operations.

* feat: implement AsyncTableBuilder (#42)

* feat: implement MirroringAsyncTable#checkAndMutate (#43)

* fix: Implement single Append and Increment as Put (#38)

* refactor: simplify SecondaryWriteErrorConsumer API (#78)

* feat: concurrent writes in MirroringTable (#79)

* test: fix failing concurrent write test (#120)

* refactor: renames and moves in RequestScheduling (#87)

* wip: handover session comments

Co-authored-by: Mateusz Walkiewicz <mwalkiewicz@unoperate.com>
Co-authored-by: Adam Czajkowski <prawilny@unoperate.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement single Increment, Append as Put in 2.x client

4 participants