feat: introduce closeAsync to Batcher by igorbernstein2 · Pull Request #1423 · googleapis/gax-java

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.

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.

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

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

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/

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

  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

+        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

+    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

+    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

-    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

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

  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

+    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

-    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

+        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

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

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

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

Comment thread gax/src/main/java/com/google/api/gax/batching/Batcher.java Outdated

Comment thread gax/src/test/java/com/google/api/gax/batching/BatcherImplTest.java Outdated

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

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

mutianf reviewed Jul 2, 2021

View reviewed changes

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

mutianf approved these changes Jul 2, 2021

View reviewed changes

add tests

acdddff

format

f13cacd

elharo reviewed Jul 2, 2021

View reviewed changes

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

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

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

Conversation

igorbernstein2 commented Jul 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

igorbernstein2 commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vam-google left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vam-google left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

igorbernstein2 commented Jul 2, 2021 •

edited

Loading