feat: introduce closeAsync to Batcher #1423

igorbernstein2 · 2021-07-02T11:58:30Z

This should allow callers to signal that they are done using a batcher without blocking their thread.
This will be used in bigtable-hbase to implement async batching cleanup

This should allow callers to signal that they are done using a batcher without blocking their thread.

gax/src/main/java/com/google/api/gax/batching/Batcher.java

gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java

There was a race condition of the BulkReadWrapper being cleaned up before the batch executor was done with it. This PR fixes the issue by making lifecycle management more explicit by scoping batch executors to a single method(List) invocation and making close explicit. This will be further cleaned up once googleapis/gax-java#1423 lands

elharo

There don't seem to be any tests for the new behavior.

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

igorbernstein2 · 2021-07-02T14:54:20Z

added async specific tests, which in combination with the existing sync close tests to cover the functionality fully

gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java

done

* fix: veneer adapter batching There was a race condition of the BulkReadWrapper being cleaned up before the batch executor was done with it. This PR fixes the issue by making lifecycle management more explicit by scoping batch executors to a single method(List) invocation and making close explicit. This will be further cleaned up once googleapis/gax-java#1423 lands * deflake test * use updated veneer for jwt fix * Revert "deflake test" This reverts commit 386ab65. * better error messaging

vam-google

I'm ok with the changes, but a few a few questions to confirm that the imlementation matches the original intentions/

gax/src/main/java/com/google/api/gax/batching/Batcher.java

vam-google · 2021-07-14T17:17:37Z

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

@@ -89,7 +90,7 @@
  private final Object flushLock = new Object();
  private final Object elementLock = new Object();
  private final Future<?> scheduledFuture;
-  private volatile boolean isClosed = false;
+  private SettableApiFuture<Void> closeFuture;


Should this remain volatile? I mean tthe referrence itself, because it seems it is checked for non-null value withotu anyu locks in the add() method below?

I dont believe so: it can only be set by a single caller thread and is read by that same thread or in a synchronized block.

But I dont see any harm in adding it, so I did

vam-google · 2021-07-14T17:20:12Z

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

+        BatchingException cause = (BatchingException) e.getCause();
+        throw new BatchingException(cause.getMessage());
+      } else {
+        throw new IllegalStateException("unexpected error closing the batcher", e.getCause());


This will loose the ExecutionException itself (with maybe some useful message there). I.e. why e.getCause() instead of just e as the second argument? Get cause will still be there as a "sub-exception".

ExecutionException doesn't provide any useful info here and leaks implementation details. The caller doesn't care that this was implemented with a future, but they do care that one of their elements failed.

vam-google · 2021-07-14T17:21:07Z

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

+    return closeFuture;
+  }
+
+  private void finishClose() {


Not sure that this deserves its own private method, as it is used only once and very short.

I inlined the method

vam-google · 2021-07-14T17:23:59Z

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

+    boolean shouldClose = false;
+
+    synchronized (flushLock) {
+      if (numOfOutstandingBatches.decrementAndGet() == 0) {


Maybe use <= 0 just in case as a safer option guaranteeing we will not jump over 0 and go into negative infinity?

I think that would create weird behavior. I dont think I want waiters on the flushLock to be notified multiple times

vam-google · 2021-07-14T17:26:33Z

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

-    if (isClosed) {
-      return;
+    try {
+      closeAsync().get();


This is a change in behavior - looks like close used to not take flushLock at all, so it was non-blocking. Now it is blocking (taking flushLock via closeAsync). If it was intentional, then ok, but just decided to point it out.

the old close impl called flush(), which called awaitAllOutstandingBatches(), which took the flush lock. So that behavior remains the same.

vam-google · 2021-07-14T17:27:12Z

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

+
+    boolean closeImmediately;
+
+    synchronized (flushLock) {


This would mean that closeAsync is still blocking potentially. The stuff under lock will execute immediatelly fast, but waiting for the lock may take arbitrarily long time. Is it Ok?

I dont think there is any code here that would hold the lock for an arbitrary amount of time. All critical sections are explicitly coded to avoid blocking indefinitely

igorbernstein2

@vam-google thanks for looking, I think I address all of the comments, PTAL

igorbernstein2 · 2021-07-14T21:33:59Z

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

@@ -89,7 +90,7 @@
  private final Object flushLock = new Object();
  private final Object elementLock = new Object();
  private final Future<?> scheduledFuture;
-  private volatile boolean isClosed = false;
+  private SettableApiFuture<Void> closeFuture;


I dont believe so: it can only be set by a single caller thread and is read by that same thread or in a synchronized block.

But I dont see any harm in adding it, so I did

igorbernstein2 · 2021-07-14T21:35:35Z

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

+    boolean shouldClose = false;
+
+    synchronized (flushLock) {
+      if (numOfOutstandingBatches.decrementAndGet() == 0) {


I think that would create weird behavior. I dont think I want waiters on the flushLock to be notified multiple times

igorbernstein2 · 2021-07-14T21:41:19Z

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

-    if (isClosed) {
-      return;
+    try {
+      closeAsync().get();


the old close impl called flush(), which called awaitAllOutstandingBatches(), which took the flush lock. So that behavior remains the same.

igorbernstein2 · 2021-07-14T21:43:29Z

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

+        BatchingException cause = (BatchingException) e.getCause();
+        throw new BatchingException(cause.getMessage());
+      } else {
+        throw new IllegalStateException("unexpected error closing the batcher", e.getCause());


ExecutionException doesn't provide any useful info here and leaks implementation details. The caller doesn't care that this was implemented with a future, but they do care that one of their elements failed.

igorbernstein2 · 2021-07-14T21:45:09Z

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

+
+    boolean closeImmediately;
+
+    synchronized (flushLock) {


I dont think there is any code here that would hold the lock for an arbitrary amount of time. All critical sections are explicitly coded to avoid blocking indefinitely

igorbernstein2 · 2021-07-14T21:48:08Z

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java

+    return closeFuture;
+  }
+
+  private void finishClose() {


I inlined the method

vam-google

LGTM

feat: introduce closeAsync to Batcher

5734f71

This should allow callers to signal that they are done using a batcher without blocking their thread.

igorbernstein2 requested review from a team as code owners July 2, 2021 11:58

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 2, 2021

elharo suggested changes Jul 2, 2021

View reviewed changes

gax/src/main/java/com/google/api/gax/batching/Batcher.java Show resolved Hide resolved

gax/src/main/java/com/google/api/gax/batching/Batcher.java Outdated Show resolved Hide resolved

gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java Outdated Show resolved Hide resolved

igorbernstein2 added 2 commits July 2, 2021 08:51

improve comments

dc7a40d

Merge branch 'master' into async-close

84e6923

igorbernstein2 mentioned this pull request Jul 2, 2021

fix: veneer adapter batching googleapis/java-bigtable-hbase#3053

Merged

elharo previously requested changes Jul 2, 2021

View reviewed changes

mutianf reviewed Jul 2, 2021

View reviewed changes

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java Show resolved Hide resolved

mutianf reviewed Jul 2, 2021

View reviewed changes

gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java Show resolved Hide resolved

mutianf approved these changes Jul 2, 2021

View reviewed changes

add tests

acdddff

format

f13cacd

elharo reviewed Jul 2, 2021

View reviewed changes

gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java Show resolved Hide resolved

gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java Show resolved Hide resolved

igorbernstein2 added 2 commits July 2, 2021 16:45

Merge branch 'master' into async-close

7ccd283

Merge branch 'master' into async-close

2305c3d

vam-google reviewed Jul 14, 2021

View reviewed changes

igorbernstein2 commented Jul 14, 2021

View reviewed changes

Merge branch 'master' into async-close

0a54ad0

vam-google approved these changes Jul 15, 2021

View reviewed changes

igorbernstein2 merged commit aab5288 into googleapis:master Jul 16, 2021

igorbernstein2 deleted the async-close branch July 16, 2021 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce closeAsync to Batcher #1423

feat: introduce closeAsync to Batcher #1423

igorbernstein2 commented Jul 2, 2021

elharo left a comment

igorbernstein2 commented Jul 2, 2021 •

edited

vam-google left a comment

vam-google Jul 14, 2021

igorbernstein2 Jul 14, 2021

vam-google Jul 14, 2021

igorbernstein2 Jul 14, 2021

vam-google Jul 14, 2021

igorbernstein2 Jul 14, 2021

vam-google Jul 14, 2021

igorbernstein2 Jul 14, 2021

vam-google Jul 14, 2021

igorbernstein2 Jul 14, 2021

vam-google Jul 14, 2021

igorbernstein2 Jul 14, 2021

igorbernstein2 left a comment

igorbernstein2 Jul 14, 2021

igorbernstein2 Jul 14, 2021

igorbernstein2 Jul 14, 2021

igorbernstein2 Jul 14, 2021

igorbernstein2 Jul 14, 2021

igorbernstein2 Jul 14, 2021

vam-google left a comment

feat: introduce closeAsync to Batcher #1423

feat: introduce closeAsync to Batcher #1423

Conversation

igorbernstein2 commented Jul 2, 2021

elharo left a comment

Choose a reason for hiding this comment

igorbernstein2 commented Jul 2, 2021 • edited

vam-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorbernstein2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google left a comment

Choose a reason for hiding this comment

igorbernstein2 commented Jul 2, 2021 •

edited