feat: concurrent writes in MirroringTable #79

mwalkiewicz · 2021-10-14T12:09:42Z

This change is

prawilny

When I started reading the code I felt like it was a bit unsould/unnatural. After I read it, my opinion changed for the better. Especially concurrentBatch() looks quite nice in my eyes (apart from using Exception[] as AtomicReference).
I still feel like it could get better, though. But the ideas I can think of how to do it seem to be a bit wishful (dropping java 7 and using lambdas, the Future approach discussed on the standup).

The status I use is "comment" rather than "request changes" in order not to block the merge on days I'm not in the office.

Reviewed 11 of 11 files at r1.
Reviewable status: 9 of 11 files reviewed, 9 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/MirroringTable.java, line 514 at r1 (raw file):

  @Override
  public void mutateRow(final RowMutations rowMutations) throws IOException {

Not done concurrently.

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

      batchWithSpan(Collections.singletonList(operation), results);
    } catch (RetriesExhaustedWithDetailsException e) {
      if (e.getCause(0) instanceof IOException) {

nitpick: I'd create an intermediate variable here.

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

          public ListenableFuture<Void> get() {
            // We are scheduling secondary batch to run concurrently.
            ListenableFuture<Void> secondaryOperationEnded =

There's a stupid thing we cannot fix: get() is used to receive an object from the Supplier. get() is also used to synchronously wait for Future. I nearly got burnt - had to double check that you didn't accidentally make the calls sequential.

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

        reservation = reservationRequest.get();
      }
    } catch (InterruptedException | ExecutionException e) {

IMO it's not clearer (but maybe I'm just used to the old place of this chunk of code).

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

  }

  public static <T> ArrayList<T> asArrayList(T... entries) {

Nit: it's unused and you can fit it into one line (I think): return new ArrayList<T>(Arrays.asList(entries))

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

      mirroringTable.batch(requests, results);
      fail("should have thrown");
    } catch (IOException ignored) {

nit: I like asserting that I catch the exception I expect (rather than something wrapped somehow).

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

        Arrays.asList(put1, put2, put3, put4, delete1, delete2, delete3, delete4);

    //           |  p1  |  p2  |  p3  |  p4  |  d1  |  d2  |  d3  |  d4

It's a bit unclear to me how primary or secondary can have 'x' in Put test when there is no 'x' in respective error. In other words: why is "{p,s}. error" needed?

Also, I don't understand where 'y' in secondary comes from.

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

    } catch (IOException ignored) {
    }

nit: I'd check (assertThat().isEqualTo()) one or two results to make sure that it works as expected.

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

              oneWaited.set(true);
            }
            bothRun.get(3, TimeUnit.SECONDS);

Nit: I'd add a single line comment explaining that this get() blocks the first thread. It took me a while to understand what's going on.

mwalkiewicz

Reviewable status: 8 of 11 files reviewed, 5 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/MirroringTable.java, line 514 at r1 (raw file):