feat: implement MirroringTable#checkAndMutate #43

prawilny · 2021-10-01T13:42:23Z

This change is

prawilny

Reviewable status: 0 of 2 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 418 at r1 (raw file):

        Supplier<CompletableFuture<Boolean>> secondaryFutureSupplier,
        Runnable flowControlReservationErrorHandler) {
      CompletableFuture<Boolean> resultFuture = new CompletableFuture<>();

I hope to get around to writing some utils class which would reduce amount of such boilerplate soon.

mwalkiewicz

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 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 312 at r1 (raw file):

      final CompletableFuture<T> primaryFuture,
      final Supplier<CompletableFuture<T>> secondaryFutureSupplier,
      final Runnable flowControlReservationErrorHandler) {

Do not piggy back changes, move them to a separate PR.

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 404 at r1 (raw file):

  private class MirroringCheckAndMutateBuilder implements CheckAndMutateBuilder {
    private final CheckAndMutateBuilder primaryBuilder;
    private final CheckAndMutateBuilder secondaryBuilder;

In fact this approach is wrong (jokes on me, I was talking about it today). We should only construct primary builder, because we only run conditional operation on primary database. On secondary we are issuing non-conditional write if primary write succeeded.

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 445 at r1 (raw file):

    @Override
    public CompletableFuture<Boolean> thenPut(Put put) {
      HBaseOperation operation = HBaseOperation.CHECK_AND_PUT;

And when we use unconditional operations on secondary database, we should use non-checked operation here and in other methods.

I also think that it would be possible to make checkAndMutate to be used like this:

return checkAndMutate(
   MirroringTable::WriteOperatoinInfo::new,
   put,
   operation,
   this.primaryBuilder::thenPut,
   secondaryTable::put
);

I don't really know if it is any prettier but it is less boilerplate.

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 563 at r1 (raw file):

  }

  private void mockCheckAndMutate() {

Instead of creating such a method you can mock those calls in setup and use latient().doReturn(primaryBuilder).when(primayrTable).checkAndMutate(any(...), any(...));.

Moreover, I think that there is something wrong with those mocks - it seems that you are passing result of primaryTable.checkAndMutate(...) to when. I think it should look like when(primaryTable).checkAndMutate(any(...), any(...)).thenReturn(primaryBuilder);.

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 582 at r1 (raw file):

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

You can use just verify(secondaryBuilder, times(1)).thenPut(put) without argument captor.

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 622 at r1 (raw file):

    when(primaryBuilder.thenPut(put)).thenReturn(CompletableFuture.completedFuture(true));
    mirroringTable.checkAndMutate("r1".getBytes(), "f1".getBytes()).thenPut(put);
    verify(secondaryBuilder, times(1)).thenPut(eq(put));

There is no need to use eq() here, you have to use it only when you are using captors or any() as some of the parameters.

prawilny

This PR is temporarily broken.
edit (via github): unbroke it

Reviewable status: all files reviewed, 7 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 312 at r1 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

Do not piggy back changes, move them to a separate PR.

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 404 at r1 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

In fact this approach is wrong (jokes on me, I was talking about it today). We should only construct primary builder, because we only run conditional operation on primary database. On secondary we are issuing non-conditional write if primary write succeeded.

Done.

mwalkiewicz

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dopiera, @mwalkiewicz, and @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 563 at r1 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

Instead of creating such a method you can mock those calls in setup and use latient().doReturn(primaryBuilder).when(primayrTable).checkAndMutate(any(...), any(...));.

Moreover, I think that there is something wrong with those mocks - it seems that you are passing result of primaryTable.checkAndMutate(...) to when. I think it should look like when(primaryTable).checkAndMutate(any(...), any(...)).thenReturn(primaryBuilder);.

Second part of the previous comment is totally wrong, ignore it please. Sorry.

prawilny

Reviewable status: all files reviewed, 8 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 432 at r1 (raw file):

                      secondaryFutureSupplier,
                      flowControlReservationErrorHandler)
                  .whenComplete(

It's really ugly. The FutureUtils PR contains a function that makes it a bit cleaner: in this case it would be something like

FutureUtils.assign(resultFuture, primaryResult, writeWithFlowControl(...))

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 445 at r1 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

And when we use unconditional operations on secondary database, we should use non-checked operation here and in other methods.

I also think that it would be possible to make checkAndMutate to be used like this:
return checkAndMutate(
   MirroringTable::WriteOperatoinInfo::new,
   put,
   operation,
   this.primaryBuilder::thenPut,
   secondaryTable::put
);
I don't really know if it is any prettier but it is less boilerplate.

Done with less method references and more method calls (I couldn't get through "method reference is ambigous" error).

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 563 at r1 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

Second part of the previous comment is totally wrong, ignore it please. Sorry.

Done.

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 582 at r1 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

You can use just verify(secondaryBuilder, times(1)).thenPut(put) without argument captor.

Done.

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 622 at r1 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

There is no need to use eq() here, you have to use it only when you are using captors or any() as some of the parameters.

Done.

mwalkiewicz

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dopiera and @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 512 at r2 (raw file):

  public void testConditionalWriteHappensWhenConditionIsMet()
      throws ExecutionException, InterruptedException {
    setupFlowControllerMock(flowController);

There is no need to mock flow controller here, it was mocked in setUp.

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 548 at r2 (raw file):

  @Test
  public void testCheckAndPut() throws ExecutionException, InterruptedException {
    setupFlowControllerMock(flowController);

setupFlowControllerMock not needed.

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 557 at r2 (raw file):

  @Test
  public void testCheckAndDelete() throws ExecutionException, InterruptedException {
    setupFlowControllerMock(flowController);

and here

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 566 at r2 (raw file):

  @Test
  public void testCheckAndMutate() throws ExecutionException, InterruptedException {
    setupFlowControllerMock(flowController);

and here

prawilny

Reviewable status: all files reviewed, 5 unresolved discussions (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 512 at r2 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

There is no need to mock flow controller here, it was mocked in setUp.

Done.

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 548 at r2 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

setupFlowControllerMock not needed.

Done.

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 557 at r2 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

and here

Done.

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 566 at r2 (raw file):

Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…

and here

Done.

mwalkiewicz

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

dopiera

Reviewable status: 0 of 3 files reviewed, 4 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 432 at r1 (raw file):

Previously, prawilny (Adam Czajkowski) wrote…

It's really ugly. The FutureUtils PR contains a function that makes it a bit cleaner: in this case it would be something like
FutureUtils.assign(resultFuture, primaryResult, writeWithFlowControl(...))

The comment, is stale, right?

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 474 at r5 (raw file):

    private AsyncRequestScheduling.ResultWithVerificationCompletion<CompletableFuture<Boolean>>
        checkAndMutate(

Don't you need keepReferenceUntilOperationCompletes here too?

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 497 at r5 (raw file):

              FutureUtils.forwardResult(
                  FutureUtils.delayedCompletedFuture(secondaryRequest.result, primaryResult),

I've got a couple of questions here:

why are we delaying the completion of the future returned to the user until after the secondary operation completes? so far we've not been waiting for them and instead logged potential failures to the faillog
even if we are delaying the future returned to the user until the secondary operation finishes, why not return the secondary result then?
is delayedCompletedFuture generic enough to sanction putting it in FutureUtils?

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 548 at r5 (raw file):

CheckAndMutateBuilder

I don't think that's right.

What if the user writes:

builer.qualifier(...).ifMatches(...).timeRange(...).thenDelete()

We want to generate a conditional mutation matching all the criteria. Your implementation will only match the last one.

prawilny

Reviewable status: 0 of 3 files reviewed, 3 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 474 at r5 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

Don't you need keepReferenceUntilOperationCompletes here too?

I think we don't need to: we already keep them in thenX() methods (X \in {Put, Delete, Mutate}). There are some tests too.

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 497 at r5 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

I've got a couple of questions here:

why are we delaying the completion of the future returned to the user until after the secondary operation completes? so far we've not been waiting for them and instead logged potential failures to the faillog

even if we are delaying the future returned to the user until the secondary operation finishes, why not return the secondary result then?

is delayedCompletedFuture generic enough to sanction putting it in FutureUtils?

I rewrote it - I think now it's easier to read (though IMO it is easier to miss the subtlety, too).

Answering in order:

seondaryRequest was poorly named. It just contained a pair (futureAfterFlowController, futureAfterVerificationEnded) because it had a dummy as a primary future passed to AsyncRequestScheduling.
secondaryResult contained just a null as a result,
I removed it. Instead I introduced another forwardResult (for the AsyncRequestScheduling.ResultWithXXX) - it's used in one more place and will be used in ac/increment-append-as-put after I rebase.

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 548 at r5 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

CheckAndMutateBuilder
I don't think that's right.

What if the user writes:
builer.qualifier(...).ifMatches(...).timeRange(...).thenDelete()
We want to generate a conditional mutation matching all the criteria. Your implementation will only match the last one.

I disagree. I added two tests to prove that I'm right (testCheckAndMutateBuilderChainingWhenNewBuilders and testCheckAndMutateBuilderChainingWhenInPlace).

Also let me give a rationale for this design (which I was pretty sure I put out somewhere): I couldn't find in the docs the guarantee that methods of this builder modify it in place and I thought that the member builder should be final.

dopiera

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 548 at r5 (raw file):

Previously, prawilny (Adam Czajkowski) wrote…

I disagree. I added two tests to prove that I'm right (testCheckAndMutateBuilderChainingWhenNewBuilders and testCheckAndMutateBuilderChainingWhenInPlace).

Also let me give a rationale for this design (which I was pretty sure I put out somewhere): I couldn't find in the docs the guarantee that methods of this builder modify it in place and I thought that the member builder should be final.

You're right, I misread it - it will work properly.

It is a bit weird, though. I think the idiom is to modify internal state and return this. I'd be very surprised if either Bigtable or HBase worked differently. I think I'd rewrite it to return this nonetheless.

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 483 at r6 (raw file):

              return;
            }
            if (primaryResult) {

I know it's an acquired taste, but I think it's better to write:

if (some weird condition) {
  fail();
  return;
}
do_the_common_case_stuff();

Rather than:

if (!some weird condition) {
  do_the_common_case_stuff();
} else {
  fail();
  return;
}

I find two reasons for that:

you have less indentation
you don't have to have the unusual scenarios in your head when reading the function - they've been handled at the beginning.

You did both approaches here, actually :) In short, I think it should be:

if (primaryError != null) {
   ...
   return;
}
if (!primaryResult) {
  ...
  return;
}
// common case

BTW, shouldn't we forward an error to the user if primary returned null?

dopiera

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 483 at r6 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

I know it's an acquired taste, but I think it's better to write:
if (some weird condition) {
  fail();
  return;
}
do_the_common_case_stuff();
Rather than:
if (!some weird condition) {
  do_the_common_case_stuff();
} else {
  fail();
  return;
}
I find two reasons for that:

you have less indentation

you don't have to have the unusual scenarios in your head when reading the function - they've been handled at the beginning.

You did both approaches here, actually :) In short, I think it should be:
if (primaryError != null) {
   ...
   return;
}
if (!primaryResult) {
  ...
  return;
}
// common case
BTW, shouldn't we forward an error to the user if primary returned null?

Or better yet - use exceptionally() and throw a meaningful exception when primaryResult == null.

prawilny

Dismissed @dopiera from a discussion.
Reviewable status: 0 of 3 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 548 at r5 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

You're right, I misread it - it will work properly.

It is a bit weird, though. I think the idiom is to modify internal state and return this. I'd be very surprised if either Bigtable or HBase worked differently. I think I'd rewrite it to return this nonetheless.

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 483 at r6 (raw file):

Previously, dopiera (Marek Dopiera) wrote…

Or better yet - use exceptionally() and throw a meaningful exception when primaryResult == null.

First of all, both the cases are normal - one when check returned true and the mutations have been applied, the other one when the check returned false and the mutations weren't applied.
Renamed the variable to remedy this.
I planned to make one big PR with change of style from whenComplete to whenComplete/thenAccept/exceptionally, but rewrote it right here on your request.

dopiera

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

The base branch was changed.

mwalkiewicz

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

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

prawilny requested review from dopiera and mwalkiewicz October 1, 2021 13:42

prawilny commented Oct 1, 2021

View reviewed changes

mwalkiewicz suggested changes Oct 1, 2021

View reviewed changes

prawilny force-pushed the ac/Builders/1 branch from 7faf2cd to 365aba1 Compare October 1, 2021 15:27

prawilny commented Oct 1, 2021

View reviewed changes

mwalkiewicz suggested changes Oct 2, 2021

View reviewed changes

prawilny force-pushed the ac/Builders/1 branch from 365aba1 to 911abd3 Compare October 5, 2021 17:34

prawilny force-pushed the ac/Builders/2 branch from b79c389 to 7e1bd85 Compare October 5, 2021 18:05

prawilny commented Oct 5, 2021

View reviewed changes

prawilny requested a review from mwalkiewicz October 5, 2021 18:06

prawilny force-pushed the ac/Builders/1 branch from 911abd3 to dcf9532 Compare October 6, 2021 11:09

prawilny force-pushed the ac/Builders/2 branch from 7e1bd85 to b9c2a6e Compare October 6, 2021 11:10

mwalkiewicz suggested changes Oct 6, 2021

View reviewed changes

prawilny commented Oct 6, 2021

View reviewed changes

prawilny requested a review from mwalkiewicz October 6, 2021 12:34

mwalkiewicz previously approved these changes Oct 8, 2021

View reviewed changes

prawilny force-pushed the ac/Builders/1 branch from dcf9532 to 9a54282 Compare October 12, 2021 16:21

prawilny force-pushed the ac/Builders/2 branch 2 times, most recently from 1544f63 to 49ee251 Compare October 12, 2021 16:45

dopiera requested changes Oct 12, 2021

View reviewed changes

prawilny force-pushed the ac/Builders/2 branch from 49ee251 to 71453d4 Compare October 13, 2021 09:50

prawilny commented Oct 13, 2021

View reviewed changes

dopiera requested changes Oct 13, 2021

View reviewed changes

prawilny force-pushed the ac/Builders/1 branch from 9a54282 to cee939c Compare October 13, 2021 16:45

prawilny force-pushed the ac/Builders/2 branch from 71453d4 to 819f4a0 Compare October 13, 2021 16:56

prawilny commented Oct 13, 2021

View reviewed changes

prawilny requested a review from dopiera October 13, 2021 17:03

dopiera previously approved these changes Oct 14, 2021

View reviewed changes

prawilny force-pushed the ac/Builders/1 branch from cee939c to be88162 Compare October 15, 2021 14:42

prawilny force-pushed the ac/Builders/2 branch from 819f4a0 to 033d9c2 Compare October 15, 2021 15:23

prawilny changed the base branch from ac/Builders/1 to master October 15, 2021 15:23

prawilny force-pushed the ac/Builders/2 branch from 033d9c2 to 9f1176f Compare October 15, 2021 15:43

checkAndMutate

207cc1d

prawilny force-pushed the ac/Builders/2 branch from 9f1176f to 207cc1d Compare October 15, 2021 15:47

mwalkiewicz approved these changes Oct 15, 2021

View reviewed changes

prawilny merged commit dd87217 into master Oct 15, 2021

prawilny deleted the ac/Builders/2 branch November 2, 2021 14:00

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

feat: implement MirroringAsyncTable#checkAndMutate (#43)

de19248

feat: implement MirroringTable#checkAndMutate #43

feat: implement MirroringTable#checkAndMutate #43

Uh oh!

Conversation

prawilny commented Oct 1, 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 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

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

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

prawilny commented Oct 1, 2021 •

edited by dopiera

Loading

prawilny left a comment •

edited

Loading