Future-proof some Guava usage by drcrallen · Pull Request #5414 · apache/druid

drcrallen · 2018-02-22T06:48:15Z

Use a Collections.emptyIterator() instead of Guava's
Change some of the guava future handling to do manual async
transforms. Guava changes transform into transformAsync by deprecating
transform in ONLY Guava 19. Then its gone in 20

* Use a java-util EmptyIterator instead of Guava's * Change some of the guava future handling to do manual async transforms. Guava changes transform into transformAsync by deprecating transform in ONLY Guava 19. Then its gone in 20

leventov · 2018-02-22T12:01:10Z

    return new Firehose()
    {
-      private Iterator<InputRow> nextIterator = Iterators.emptyIterator();
+      private Iterator<InputRow> nextIterator = EmptyIterator.instance();


Why not Collections.emptyIterator()?

hah, doh. Fixed

leventov · 2018-02-22T12:03:09Z

                    "dataSegment for segmentId[%s]",
-                    segmentWithState.getSegmentIdentifier())
+                    segmentWithState.getSegmentIdentifier()
+                     )


Bad formatting

Pretty now!

leventov · 2018-02-22T12:03:35Z

        committerSupplier.get(),
        Collections.singletonList(sequenceName)
    );
+    final SettableFuture<SegmentsAndMetadata> handoffDoneFuture = SettableFuture.create();


Could this boilerplate be extracted in some method?

in theory yes, but the different places this kind of style is used have slightly different usage patterns, and I didn't want to spend the mental bandwidth balancing between java futures and guava futures (that are also viable across a wide swath of Guava version) that also fit the different use cases. So I just wrote things specific for each use case.

I filed #5415

leventov · 2018-02-22T12:03:46Z

-        pushInBackground(null, segmentIdentifierList),
-        this::dropInBackground
-    );
+    final CompletableFuture<SegmentsAndMetadata> chainEndFuture = new CompletableFuture<>();


If SettableFuture is used instead of CompletableFuture, this code becomes isomorphic to the code in AppenderatorDriverRealtimeIndexTask and a common method transform() (replacement of Futures.transform()) could be extracted.

leventov · 2018-02-22T12:05:20Z

                <artifactId>guice-multibindings</artifactId>
                <version>${guice.version}</version>
            </dependency>
+            <dependency>


What is this? Why this is needed?

Future versions of guava fail to pull this in. 19 specifically if I recall. This causes builds to fail for guava 19 without it.

leventov · 2018-02-22T12:12:33Z

-        pushInBackground(wrapCommitter(committer), theSegments),
-        (AsyncFunction<SegmentsAndMetadata, SegmentsAndMetadata>) segmentsAndMetadata -> publishInBackground(
-            segmentsAndMetadata,
+    final SettableFuture<SegmentsAndMetadata> publishCompleted = SettableFuture.create();


leventov · 2018-02-22T12:12:40Z

-        publish(publisher, committer, sequenceNames),
-        this::registerHandoff
-    );
+    final SettableFuture<SegmentsAndMetadata> publishDoneFuture = SettableFuture.create();


drcrallen · 2018-02-22T16:29:45Z

@leventov addressed comments

leventov

I don't actually see Futures.transform() is gone. It seems to me that it's deprecated, but not gone.
Please prohibit methods that you avoid using forbidden-apis, as @b-slim suggested in the other PR.

leventov · 2018-02-22T20:40:24Z

-        this::registerHandoff
-    );
+    final SettableFuture<SegmentsAndMetadata> publishDoneFuture = SettableFuture.create();
+    final ListenableFuture<SegmentsAndMetadata> publishFuture = publish(publisher, committer, sequenceNames);


Unless there is a bug, as it is currently written, there is no point in creation of publishDoneFuture and publishFuture could be just returned from this method.

bug, fixing

leventov · 2018-02-22T20:43:17Z

-        pushInBackground(null, segmentIdentifierList),
-        this::dropInBackground
-    );
+    final CompletableFuture<SegmentsAndMetadata> chainEndFuture = new CompletableFuture<>();


If SettableFuture is used instead of CompletableFuture, this code becomes isomorphic to the code in AppenderatorDriverRealtimeIndexTask and a common method transform() (replacement of Futures.transform()) could be extracted.

drcrallen · 2018-02-23T01:17:12Z

@leventov I thiiiiiink I put in something like what you are asking for. Let me know if this is close

leventov · 2018-02-23T16:48:47Z

@drcrallen please respond to this comment - #5414 (review)

drcrallen · 2018-02-24T01:30:31Z

I dis but pushed to the wrong branch. Fixing.

drcrallen · 2018-02-24T02:40:19Z

@leventov 7e34fd3 is in the correct branch now

leventov · 2018-02-26T14:20:15Z

@@ -0,0 +1 @@
+com.google.common.util.concurrent.Futures.transform(com.google.common.util.concurrent.ListenableFuture<I>, com.google.common.util.concurrent.AsyncFunction<? super I,? extends O>) @ Use io.druid.java.util.common.concurrent.ListenableFutures.transform


I don't think it needs to be a different file, druid-forbidden-apis.txt already forbids Guava APIs.

Forbid Iterators.emptyIterator()

And still -

I don't actually see Futures.transform() is gone. It seems to me that it's deprecated, but not gone.

And won't be removed, if Guava keeps the current policy of non-removing APIs.

The async one is gone, or at least changed to a new method name, I think asyncTransform. The method for the non-async one is still there and unchanged.

I see now. Could you please

Match the method name in ListenableFutures with the present name in Guava library.

Add comments to the method, and to the entry in forbidden-apis file (it supports comments with #), that they should be removed, with switch back to Guava, as soon as the minimum supported Guava version becomes 19.0.

That sounds good

leventov · 2018-02-26T14:22:52Z

      subHashTable2Buffer = subHashTable2Buffer.slice();

-      subHashTableBuffers = new ByteBuffer[] {subHashTable1Buffer, subHashTable2Buffer};
+      subHashTableBuffers = new ByteBuffer[]{subHashTable1Buffer, subHashTable2Buffer};


Could you add a checkstyle rule (I think it could be a simple regex) to catch such things automatically?

Outside scope of this PR

leventov · 2018-02-27T11:58:16Z

   * <ul>
   * <li>null, boolean, int, long, float, double, string, Records, Enums, Maps, Fixed -> String, using String.valueOf</li>
   * <li>bytes -> Arrays.toString() or new String if binaryAsString is true</li>
-   * <li>Arrays -> List&lt;String&gt;, using Lists.transform(&lt;List&gt;dimValue, TO_STRING_INCLUDING_NULL)</li>


It shouldn't be changed, as well as all other comments, changed in this commit.

sigh intellij... fixed

leventov · 2018-02-27T11:58:19Z

+  /**
+   * Guava 19 changes the Futures.transform signature so that the async form is different. This is here as a
+   * compatability layer until such a time as druid only supports Guava 19 or later, in which case
+   * Futrues.transformAsync should be used


leventov · 2018-02-27T11:59:24Z

 com.google.common.collect.MapMaker @ Create java.util.concurrent.ConcurrentHashMap directly
-com.google.common.collect.Maps#newConcurrentMap() @ Create java.util.concurrent.ConcurrentHashMap directly
+com.google.common.collect.Maps#newConcurrentMap() @ Create java.util.concurrent.ConcurrentHashMap directly
+com.google.common.util.concurrent.Futures.transform(com.google.common.util.concurrent.ListenableFuture<I>, com.google.common.util.concurrent.AsyncFunction<? super I,? extends O>) @ Use io.druid.java.util.common.concurrent.ListenableFutures.transformAsync


Also forbid Iterators.emptyIterator()

leventov · 2018-02-27T16:49:12Z

 com.google.common.collect.MapMaker @ Create java.util.concurrent.ConcurrentHashMap directly
 com.google.common.collect.Maps#newConcurrentMap() @ Create java.util.concurrent.ConcurrentHashMap directly
-com.google.common.util.concurrent.Futures.transform(com.google.common.util.concurrent.ListenableFuture<I>, com.google.common.util.concurrent.AsyncFunction<? super I,? extends O>) @ Use io.druid.java.util.common.concurrent.ListenableFutures.transformAsync
+com.google.common.util.concurrent.Futures.transform(com.google.common.util.concurrent.ListenableFuture<I>, com.google.common.util.concurrent.AsyncFunction<? super I,? extends O>) @ Use io.druid.java.util.common.concurrent.ListenableFutures#transformAsync


This line uses different method referencing syntax than the other lines (Futures.transform vs. Maps#newConcurrentMap). Did you check that this prohibition works by trying to add a use of prohibited method to code and then mvn install to see if the build fails?

drcrallen · 2018-03-12T14:21:52Z

@leventov any other comments here?

gianm · 2018-03-13T04:57:15Z

+public class ListenableFutures
+{
+  /**
+   * Guava 19 changes the Futures.transform signature so that the async form is different. This is here as a


Is this code copied from Guava? If so, the comment should say so. If not, the comment should also say so, since people might think it was anyway.

gianm · 2018-03-13T04:59:33Z

-        this::registerHandoff
-    );
+    return ListenableFutures
+        .transformAsync(


this line break doesn't seem necessary. It is a small thing but I still feel compelled to mention it.

gianm · 2018-03-13T05:00:11Z

-        this::dropInBackground
-    );
+    final ListenableFuture<SegmentsAndMetadata> future = ListenableFutures
+        .transformAsync(


this line break doesn't seem necessary. It is a small thing but I still feel compelled to mention it.

gianm · 2018-03-13T05:01:35Z

 /**
 * This class is specialized for streaming ingestion. In streaming ingestion, the segment lifecycle is like:
- *
+ * <p>


I think we avoid these <p> tags since the linter doesn't like them. Try turning off "Generate p on empty lines" in the JavaDoc section of your code style pane.

gianm · 2018-03-13T05:02:21Z

+      synchronized (segments) {
+        sequenceNames.forEach(segments::remove);
+      }
+      final SettableFuture<SegmentsAndMetadata> future = SettableFuture.create();


Why change this to an async transform that generates a useless future, instead of keeping it as a normal transform?

I'm not following, what do you mean by a "normal transform" here?

I think I fixed this, but please check

By "normal transform" I meant one that uses a regular Function rather than AsyncFunction. The new diff looks good to me now.

leventov · 2018-03-13T14:58:13Z

 com.google.common.collect.MapMaker @ Create java.util.concurrent.ConcurrentHashMap directly
-com.google.common.collect.Maps#newConcurrentMap() @ Create java.util.concurrent.ConcurrentHashMap directly
+com.google.common.collect.Maps#newConcurrentMap() @ Create java.util.concurrent.ConcurrentHashMap directly
+com.google.common.util.concurrent.Futures.transform(com.google.common.util.concurrent.ListenableFuture<I>, com.google.common.util.concurrent.AsyncFunction<? super I,? extends O>) @ Use io.druid.java.util.common.concurrent.ListenableFutures#transformAsync


It's still different from other signatures. I'm not sure which syntax is correct, so please check that those prohibitions work

https://github.com/policeman-tools/forbidden-apis/wiki/SignaturesSyntax I'm trying this out more to see.

If the signature is this

com.google.common.util.concurrent.Futures#transform(com.google.common.util.concurrent.ListenableFuture, com.google.common.util.concurrent.AsyncFunction) @ Use io.druid.java.util.common.concurrent.ListenableFutures#transformAsync

And I change one of the lines to this

return Futures.transform( publishFuture, (com.google.common.util.concurrent.AsyncFunction<? super SegmentsAndMetadata, ? extends SegmentsAndMetadata>) sam -> { //(Function<? super SegmentsAndMetadata, ? extends SegmentsAndMetadata>) sam -> { synchronized (segments) { sequenceNames.forEach(segments::remove); } final SettableFuture<SegmentsAndMetadata> samfuture = SettableFuture.create(); samfuture.set(sam); return samfuture; } );

then maven gives me this:

[ERROR] Forbidden method invocation: com.google.common.util.concurrent.Futures#transform(com.google.common.util.concurrent.ListenableFuture, com.google.common.util.concurrent.AsyncFunction) [Use io.druid.java.util.common.concurrent.ListenableFutures#transformAsync] [ERROR] in io.druid.segment.realtime.appenderator.StreamAppenderatorDriver (StreamAppenderatorDriver.java:278) [ERROR] Scanned 1005 (and 1148 related) class file(s) for forbidden API invocations (in 0.63s), 1 error(s).

So I'll make a change here that is validated to work. I could have sworn I tried the one in this comment before, but apparently not. Now I have a validated one though.

…mpatProblems

…wiki/SignaturesSyntax

drcrallen · 2018-03-19T17:42:19Z

@gianm / @leventov any other comments?

drcrallen · 2018-03-20T13:53:27Z

Thanks!

Future-proof some Guava usage

f4deddf

* Use a java-util EmptyIterator instead of Guava's * Change some of the guava future handling to do manual async transforms. Guava changes transform into transformAsync by deprecating transform in ONLY Guava 19. Then its gone in 20

leventov reviewed Feb 22, 2018

View reviewed changes

Use Collections.emptyIterator()

a54c7e5

drcrallen mentioned this pull request Feb 22, 2018

Async future handling needs a guava friendly way we can maintain. #5415

Closed

Pretty formatting

6f8cd33

drcrallen added the Compatibility label Feb 22, 2018

leventov reviewed Feb 22, 2018

View reviewed changes

Charles Allen added 2 commits February 22, 2018 17:00

Make listenable future transforms a thing in default druid

6491e47

Format fix

b9cc2ab

Add forbidden guava apis

7e34fd3

drcrallen mentioned this pull request Feb 24, 2018

Guava-free Same Thread Executor #5413

Closed

leventov reviewed Feb 26, 2018

View reviewed changes

Make the ListenableFutrues.transformAsync have comments

f831dcb

leventov reviewed Feb 27, 2018

View reviewed changes

Charles Allen added 4 commits February 27, 2018 08:37

Undo intellij bad pattern matching in comments

ac6caa5

Futrues --> Futures

5b696a9

Add empty iterators forbidding

3240728

Fix extra A

fa03a75

leventov reviewed Feb 27, 2018

View reviewed changes

Correct method signature

13692b8

gianm reviewed Mar 13, 2018

View reviewed changes

leventov reviewed Mar 13, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into guava/lessGuavaCo…

72f53df

…mpatProblems

Charles Allen added 3 commits March 19, 2018 10:21

Address review comments

cc5d60e

Finish Gian review comments

3bbe113

Proper syntax from https://github.com/policeman-tools/forbidden-apis/…

716c631

…wiki/SignaturesSyntax

leventov approved these changes Mar 19, 2018

View reviewed changes

gianm approved these changes Mar 20, 2018

View reviewed changes

gianm merged commit 58f110f into apache:master Mar 20, 2018

drcrallen deleted the guava/lessGuavaCompatProblems branch March 20, 2018 16:12

drcrallen added this to the 0.13.0 milestone Apr 6, 2018

		@@ -0,0 +1 @@
		com.google.common.util.concurrent.Futures.transform(com.google.common.util.concurrent.ListenableFuture<I>, com.google.common.util.concurrent.AsyncFunction<? super I,? extends O>) @ Use io.druid.java.util.common.concurrent.ListenableFutures.transform No newline at end of file

Conversation

drcrallen commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

drcrallen commented Feb 22, 2018

Uh oh!

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

drcrallen commented Feb 23, 2018

Uh oh!

leventov commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drcrallen commented Feb 24, 2018

Uh oh!

drcrallen commented Feb 24, 2018

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!

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!

drcrallen commented Feb 22, 2018 •

edited

Loading

leventov commented Feb 23, 2018 •

edited

Loading