feat: 2.x reads sampling #114

mwalkiewicz · 2021-10-25T11:02:39Z

This change is

prawilny · 2021-10-27T12:12:58Z

I allowed myself to force-push after rebasing the code.

prawilny

Looks okay. I left some nitpicks, though.

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 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/BatchHelpers.java, line 284 at r1 (raw file):

          List<ActionType> operations,
          Object[] results,
          ReadSampler readSampler,

unused

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

        ReflectionConstructor.construct(
            this.configuration.mirroringOptions.writeErrorConsumerClass, failedWritesLogger);

I'm the one who introduced this line change (during the rebase), I think.

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

              if (failedSuccessfulSplit.successfulOperations.isEmpty()) {
                // Two cases
                // - either everything failed - all results were instances of Throwable and we

This comment needs to be a bit clearer IMO:
eg.
"all results were instances" => "all primary results were instances"
"were excluded" => "were excluded from the split"
"forward primary results to user results" => "forward primary results to a list returned to user"

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

                // already completed exceptionally result futures with errorHandler passed to
                // waitForAllWithErrorHandler, or
                // - reads were successful but were excluded due to sampling and we should forward

nit(?): "or" after "-" ()

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

  }

  public <T> CompletableFuture<Void> allOfList(List<CompletableFuture<T>> list) {

Maybe it should (ideally) even be in FutureUtils? Still, probably it's not worth the time.

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

  }

  @Test(expected = UnsupportedOperationException.class) // Disable until scanner is implemented

Should we create an issue to remember about it?

The base branch was changed.

mwalkiewicz

Reviewable status: 3 of 7 files reviewed, 5 unresolved discussions (waiting on @prawilny)

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