Enhance SearchFilter.QueryType and FailedFuture #396

kevinhinterlong · 2017-06-13T16:08:41Z

This adds some minor enhancements to FailedFuture and allows for a QueryType to be found from its serialized name.

QubitPi · 2017-06-13T22:00:06Z

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/SearchFilter.java

+        /**
+         * Get the QueryType enum fromType it's search type.
+         * @param type  Type of the query type (for serialization)
+         * @return the enum QueryType


Empty lines before @return

QubitPi · 2017-06-13T22:00:29Z

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/SearchFilter.java

+
+        /**
+         * Get the QueryType enum fromType it's search type.
+         * @param type  Type of the query type (for serialization)


Empty lines before @param

QubitPi · 2017-06-13T22:01:08Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/CompletedFuture.java

@@ -0,0 +1,60 @@
+// Copyright 2016 Yahoo Inc.


asifmansoora

This PR looks good to me 👍

QubitPi

CHANGELOG needed; make sure to explain why you add CompletedFuture and delete the other

QubitPi · 2017-06-17T00:36:06Z

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/SearchFilter.java

@@ -1,4 +1,4 @@
-// Copyright 2016 Yahoo Inc.
+// Copyright 2017 Yahoo Inc.


Looks like this file is there since 2016 (we put based on when file is initialized)

QubitPi · 2017-06-17T00:46:40Z

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/SearchFilter.java

+         *
+         * @return the enum QueryType
+         */
+        public static QueryType fromType(String type) {


There is a standard way(simpler and more readable) of achieving your purpose. Example will be https://github.com/yahoo/fili/blob/master/fili-core/src/main/java/com/yahoo/bard/webservice/config/CacheFeatureFlag.java or http://grepcode.com/file_/repo1.maven.org/maven2/javax.ws.rs/jsr311-api/1.1/javax/ws/rs/core/Response.java/?v=source

Instead of fromType, you have

public String getType() { return type; }

And you check type by having "contains".equals(QueryType.Contains.getType()). That's how we check type of enum (as the two link example does, as well)

I'm not sure I understand what you mean. Do you want me to enumerate each possibility to find which type the enum is instead of using the loop?

From the second example you gave they did this which is nearly identical to the way I checked the type.

public static Status fromStatusCode(final int statusCode) { for (Status s : Status.values()) { if (s.code == statusCode) { return s; } } return null; }

QubitPi · 2017-06-21T00:48:23Z

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/SearchFilter.java

+        /**
+         * Get the QueryType enum from it's search type.
+         *
+         * @param type  The type of query (i.e. "insensitive_contains")


The type of query in String (i.e. "insensitive_contains")

Same as above

QubitPi · 2017-06-21T00:53:51Z

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/SearchFilter.java

@@ -42,6 +42,22 @@
        QueryType(String type) {
            this.type = type;
        }
+
+        /**
+         * Get the QueryType enum from it's search type.


Get the QueryType enum from it's search type in String

I think this is clear from context as you can only pass in a String to the method.

QubitPi · 2017-06-21T00:55:27Z

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/SearchFilter.java

+         *
+         * @return the enum QueryType
+         */
+        public static QueryType fromType(String type) {


final String type

What's the reason for adding final?
I've been told in previous PRs to remove final from parameters

QubitPi · 2017-06-21T00:55:37Z

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/SearchFilter.java

+                    return queryType;
+                }
+            }
+            throw new IllegalArgumentException("No query type corresponds to " + type);


If you decide to go this way, return 'null' on non-maching QueryType.

QubitPi · 2017-06-21T01:10:23Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/CompletedFuture.java

+            throw new ExecutionException(throwable);
+        } else {
+            return item;
+        }


else is not necessary

QubitPi · 2017-06-21T01:11:20Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/CompletedFuture.java

+
+    /**
+     * Creates a completed future with an item to return or
+     * if a nonnull throwable is passed it it will throw an


passed it -> provided ?

QubitPi · 2017-06-21T01:12:45Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/CompletedFuture.java

+    /**
+     * Creates a completed future with an item to return or
+     * if a nonnull throwable is passed it it will throw an
+     * execution exception.


{@link java.util.concurrent.ExecutionException} gives better reference

QubitPi · 2017-06-21T01:13:33Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/CompletedFuture.java

+    private final Throwable throwable;
+
+    /**
+     * Creates a completed future with an item to return or


an item to return on on {@link #get()}

I think this is clear from the context and adding this could make it unclear what happens when {@link #get(long, TimeUnit)} is called.

QubitPi · 2017-06-21T01:15:48Z

...re/src/test/java/com/yahoo/bard/webservice/models/druid/client/impl/TestDruidWebService.java

@@ -90,7 +90,7 @@ public TestDruidWebService() {
    /**
     * If TestDruidWebService#throwable is set, invokes the failure callback.  Otherwise invokes success or failure
     * dependent on whether TestDruidWebService#statusCode equals 200.
-     * Note that since this doesn't send requests to druid all the responses will be null or {@link FailedFuture}.
+     * Note that since this doesn't send requests to druid all the responses will be null or {@link CompletedFuture}.


Since this class is in a different package

{@link com.yahoo.bard.webservice.util.CompletedFuture}

It's available from context because of

import com.yahoo.bard.webservice.util.CompletedFuture;

do you want me to add the fully qualified name?

This is not for readability. The reason to put complete path is for Javadoc plugin to be able to recognize and inject HTML link that points to the correct class.

The javadoc plugin is able to correctly recognize this and inject the html link in other classes I've tried (like in CompletedFuture). Did you generate the javadoc and it didn't work correctly?

QubitPi · 2017-06-21T01:21:28Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/CompletedFuture.java

+     * @param item  The item to return when called.
+     * @param throwable  A non null throwable if an exception should be thrown.
+     */
+    public CompletedFuture(E item, Throwable throwable) {


I see this constructor is called everywhere with item = null, like new CompletedFuture<>(null, throwable). This makes code less readable, because readers don't know that null means. A better approach would be

public CompletedFuture(E item, Throwable throwable) { this.item = item; this.throwable = throwable; } public CompletedFuture(Throwable throwable) { this(null, throwable); }

That way, you do new CompletedFuture<>(throwable).

I've decided to replace this with Either and give two ways to construct it which make it clear what it will do.

archolewa · 2017-07-07T15:52:02Z

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/SearchFilter.java

+         *
+         * @return the enum QueryType.
+         */
+        public static QueryType fromType(String type) {


I would argue that this should be returning an Optional instead of a null, or throwing an IllegalArgumentException if the type is not a recognized QueryType.

Returning an Optional makes it clear to people using the code that they may not get a QueryType back, and need to deal with that case. This makes sense if it's totally valid to have a string type that cannot be translated to a QueryType.

The second case assumes that it's an error if we get an unrecognized type.

I'm assuming that it's an error to receive an unrecognized type, but I don't know yet what it's meant to be used for, so I can't say for certain.

@QubitPi Are you fine with me reverting to the IllegalArgumentException/returning optional?

For context, this was made because SearchFilter doesn't have a reference to its QueryType and I have to process a SearchFilter differently for each case (and if not found, throw an exception).

Additionally, if SearchFilter must be one of those QueryType then it should probably be verified in the constructor that accepts a String queryType.

@archolewa Agree

QubitPi

I promise a deep review on CompletedFuture later today

QubitPi · 2017-07-18T02:00:03Z

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/SearchFilter.java

+         *
+         * @return the enum QueryType.
+         */
+        public static QueryType fromType(String type) {


@archolewa Agree

QubitPi · 2017-07-18T02:13:19Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/CompletedFuture.java

+     * Private constructor - all methods static.
+     */
+    private CompletedFuture() {
+


//Intentionally left blank.

QubitPi · 2017-07-18T16:05:02Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/CompletedFuture.java

+    /**
+     * Creates a completed {@link java.util.concurrent.Future} which will return this item.
+     *
+     * @param item  The item to return when called.


QubitPi · 2017-07-18T16:05:09Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/CompletedFuture.java

+     * Creates a completed {@link java.util.concurrent.Future} which will return this item.
+     *
+     * @param item  The item to return when called.
+     * @param <E>  The type to be returned by this future.


QubitPi · 2017-07-18T16:05:31Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/CompletedFuture.java

+     * @param item  The item to return when called.
+     * @param <E>  The type to be returned by this future.
+     *
+     * @return a completed {@link CompletableFuture} which will successfully return an item.


no period and all others as well

QubitPi · 2017-07-19T10:30:09Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/CompletedFuture.java

+     *
+     * @return a completed {@link CompletableFuture} which will successfully return an item.
+     */
+    public static <E> CompletableFuture<E> returning(E item) {


Where is this method used?

I was using this in #430 but it's no longer needed and can be deleted if that makes sense.

Let's remove it if it's no longer needed

QubitPi

Getting there. Code looks very good!

QubitPi · 2017-07-19T23:28:13Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/CompletedFuture.java

+     *
+     * @return a completed {@link CompletableFuture} which will successfully return an item.
+     */
+    public static <E> CompletableFuture<E> returning(E item) {


Let's remove it if it's no longer needed

QubitPi · 2017-07-19T23:30:45Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/CompletedFuture.java

+     * Private constructor - all methods static.
+     */
+    private CompletedFuture() {
+        //Intentionally left blank.


Space after //

QubitPi · 2017-07-20T16:35:36Z

...re/src/test/java/com/yahoo/bard/webservice/models/druid/client/impl/TestDruidWebService.java

@@ -90,7 +90,7 @@ public TestDruidWebService() {
    /**
     * If TestDruidWebService#throwable is set, invokes the failure callback.  Otherwise invokes success or failure
     * dependent on whether TestDruidWebService#statusCode equals 200.
-     * Note that since this doesn't send requests to druid all the responses will be null or {@link FailedFuture}.
+     * Note that since this doesn't send requests to druid all the responses will be null or {@link CompletedFuture}.


This is not for readability. The reason to put complete path is for Javadoc plugin to be able to recognize and inject HTML link that points to the correct class.

QubitPi · 2017-07-20T16:36:11Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/CompletedFuture.java

+     * @param item  The item to return when called
+     * @param <E>  The type to be returned by this future
+     *
+     * @return a completed {@link CompletableFuture} which will successfully return an item.


no period at end

QubitPi · 2017-07-20T16:39:01Z

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/SearchFilter.java

+        /**
+         * Get the QueryType enum from it's search type.
+         *
+         * @param type  The type of query (i.e. "insensitive_contains")


(i.e. "insensitive_contains") indicates this is the only QueryType? I would remove it.

QubitPi · 2017-07-20T16:40:23Z

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/SearchFilter.java

@@ -78,18 +95,26 @@ private SearchFilter(Dimension dimension, String type, String value) {
        return query;
    }

+    private String getQueryType() {
+        return query.get("type");


type is used multiple times. It's more maintainable to have it as a static final String.

QubitPi · 2017-07-20T16:40:29Z

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/filter/SearchFilter.java

+    }
+
+    private String getQueryValue() {
+        return query.get("value");


QubitPi · 2017-07-20T16:41:16Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/CompletedFuture.java

+/**
+ * A utility class to create futures which have already been completed
+ * and will return as soon as it is called.
+ *


QubitPi · 2017-07-20T16:41:42Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/CompletedFuture.java

+
+/**
+ * A utility class to create futures which have already been completed
+ * and will return as soon as it is called.


can be move to the previous line

QubitPi · 2017-07-20T16:55:52Z

fili-core/src/main/java/com/yahoo/bard/webservice/util/CompletedFuture.java

+     */
+    public static <E> CompletableFuture<E> throwing(Throwable throwable) {
+        CompletableFuture<E> completedFuture = CompletableFuture.supplyAsync(() -> null);
+        completedFuture.completeExceptionally(throwable);


@michael-mclawhorn @garyluoex @archolewa I'm little concerned about this statement. According to its doc

"If not already completed, causes invocations of get() and related methods to throw the given exception."

Looks like this method wants to throw exception on complete instead (Or Javadoc of this method is simply wrong?).

- This replaces the FailedFuture by being able to be return a value or throw an exception

- Clarify usage of CompletedFuture by limiting how it can be created - add two helper methods to SearchFilter

kevinhinterlong added NEED 2 REVIEWS REVIEWABLE labels Jun 13, 2017

QubitPi suggested changes Jun 13, 2017

View reviewed changes

QubitPi self-assigned this Jun 13, 2017

QubitPi added the NEED CHANGES label Jun 13, 2017

asifmansoora approved these changes Jun 14, 2017

View reviewed changes

yahoo deleted a comment Jun 14, 2017

kevinhinterlong added NEED 1 REVIEW and removed NEED 2 REVIEWS labels Jun 15, 2017

QubitPi added the NEED REBASE label Jun 17, 2017

QubitPi suggested changes Jun 17, 2017

View reviewed changes

kevinhinterlong force-pushed the smallCoreEnhancement branch from 1d49b17 to 273b31f Compare June 19, 2017 13:56

yahoo deleted a comment Jun 19, 2017

kevinhinterlong removed the NEED REBASE label Jun 19, 2017

QubitPi suggested changes Jun 21, 2017

View reviewed changes

QubitPi added the NEED REBASE label Jun 21, 2017

kevinhinterlong force-pushed the smallCoreEnhancement branch 2 times, most recently from b715d5c to bec1580 Compare June 21, 2017 14:28

yahoo deleted a comment Jun 21, 2017

archolewa requested changes Jul 7, 2017

View reviewed changes

QubitPi suggested changes Jul 18, 2017

View reviewed changes

yahoo deleted a comment Jul 18, 2017

QubitPi suggested changes Jul 19, 2017

View reviewed changes

QubitPi suggested changes Jul 20, 2017

View reviewed changes

yahoo deleted a comment Jul 24, 2017

QubitPi approved these changes Jul 28, 2017

View reviewed changes

archolewa approved these changes Jul 28, 2017

View reviewed changes

archolewa removed the NEED 1 REVIEW label Jul 28, 2017

archolewa removed the NEED CHANGES label Jul 28, 2017

kevinhinterlong added 11 commits July 28, 2017 16:57

Add ability to find QueryType from it's serialized string.

f163734

Add a completedFuture which returns or throws as soon as it's called

60ec97a

- This replaces the FailedFuture by being able to be return a value or throw an exception

Delete FailedFuture

ca40b88

Address comments

1dfa413

Change Copyright year

d61653b

Add changelog entry showing changes. Fix year

9abe96d

Addressed comments

15886c5

- Clarify usage of CompletedFuture by limiting how it can be created - add two helper methods to SearchFilter

Only use utility functions and java core CompletableFuture

eeae27b

address comments

7378c90

Partially address comments

fa73c7e

Finish addressing comments

d7c486d

kevinhinterlong force-pushed the smallCoreEnhancement branch from 4c92039 to d7c486d Compare July 28, 2017 21:57

kevinhinterlong removed the NEED REBASE label Jul 28, 2017

yahoo deleted a comment Jul 28, 2017

kevinhinterlong added MERGEABLE and removed REVIEWABLE labels Jul 28, 2017

QubitPi merged commit 90fcf47 into yahoo:master Jul 28, 2017

kevinhinterlong deleted the smallCoreEnhancement branch August 8, 2017 14:52

		@@ -1,4 +1,4 @@
		// Copyright 2016 Yahoo Inc.
		// Copyright 2017 Yahoo Inc.

Enhance SearchFilter.QueryType and FailedFuture #396

Enhance SearchFilter.QueryType and FailedFuture #396

Conversation

kevinhinterlong commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asifmansoora left a comment

Choose a reason for hiding this comment

QubitPi 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

QubitPi Jun 21, 2017 • edited Loading

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

QubitPi Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QubitPi Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

kevinhinterlong Jun 21, 2017 • edited Loading

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

QubitPi 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

QubitPi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QubitPi Jul 20, 2017 • edited Loading

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

QubitPi Jun 21, 2017 •

edited

Loading

QubitPi Jul 20, 2017 •

edited

Loading

QubitPi Jun 21, 2017 •

edited

Loading

kevinhinterlong Jun 21, 2017 •

edited

Loading

QubitPi Jul 20, 2017 •

edited

Loading