DRILL-6633: Replace usage of Guava classes by JDK ones #1397

vvysotskyi · 2018-07-25T09:19:11Z

In the PR for DRILL-6422 was decided to create a separate Jira (DRILL-6633) for changes connected with replacing usage of some Guava classes by JDK ones.

In this PR revised most of the Guava classes and replaced their usage where possible.

arina-ielchiieva · 2018-07-25T09:23:25Z

@vdiravka could you please review?

kkhatua · 2018-07-25T18:23:36Z

@arina-ielchiieva / @vvysotskyi The changes look trivial, but 682 files are a lot to review! Should this PR be broken up in parts?

vvysotskyi · 2018-07-26T13:56:04Z

@kkhatua, I don't see a reason for splitting the PR, even splitting changes for modules where they were made. Either single or multiple PRs should be reviewed in any case.

Since almost all of the changes are trivial, it wouldn't be a problem to do a cursory review.

vdiravka

Drill project is bigger than I imagined :)

vdiravka · 2018-07-30T16:52:13Z

common/src/main/java/org/apache/drill/common/KerberosUtil.java

@@ -57,9 +57,9 @@ public static String getPrincipalFromParts(final String primary, final String in
  public static String[] splitPrincipalIntoParts(final String principal) {
    final String[] components = principal.split("[/@]");
    checkState(components.length == 3);


Consider the substitution for Preconditions.checkState() is Validate.validState() from org.apache.commons commons-lang3 for all places in the project

I think we should leave it as it is since the goal of this PR is to replace guava usage by JDK where it is possible, but there is no sense to replace usage of one library by another.

@vdiravka What is wrong with using guava Preconditions?

Nothing wrong with Guava Preconditions and with Apache Commons as well.
IMO the method names in Apache Commons are more clear.

But I've wrongly supposed that the purpose was to get rid Guava as much as possible.

We can leave Guava Preconditions, since it is already written and usage of it is more common.

vdiravka · 2018-07-30T17:42:58Z

common/src/main/java/org/apache/drill/common/collections/MapWithOrdinal.java

-        }
-      }));
+      return primary.entrySet().stream()
+          .map((Function<Entry<K, Entry<Integer, V>>, Entry<K, V>>) entry ->


Could you remove the type of lambda expession?

Thanks for pointing this, removed.

vdiravka · 2018-08-02T15:17:15Z

common/src/test/java/org/apache/drill/test/SubDirTestWatcher.java

    this.createAtBeginning = createAtBeginning;
    this.deleteAtEnd = deleteAtEnd;
-    this.subDirs = Preconditions.checkNotNull(subDirs);
+    this.subDirs = Objects.requireNonNull(subDirs);

    Preconditions.checkArgument(!subDirs.isEmpty(), "The list of subDirs is empty.");


Consider the substitution for Preconditions.checkArgument() is Validate.isTrue() from org.apache.commons commons-lang3.

See my previous comment

While in this place Preconditions.checkNotNull() and Objects.requireNonNull() provide the same functionality, in general guava Preconditions supports template error message that Objects.requireNonNull does not support. I'd recommend keeping Preconditions.checkNotNull for consistency. IMO, it is necessary to remove deprecated guava classes, not all references to guava where similar functionality is provided by JDK.

Objects.requireNonNull is introduced mostly for Streams, but not for replacing Guava Preconditions.
From other hand they have the same implementation. Except Preconditions.checkNotNull() overloaded method with NPE error message formatting and Guava recommends to use Preconditions, see Guava master.

Looks like Calcite project replaces these methods, see Calcite master, but Apex doesn't replace them.

Since we stay with Guava it could be reasonably to follow their documentation.
But it is not critical, so I will leave the decision for you.

I prefer using JDK things instead of third-party libraries, so I leave code with Objects.requireNonNull() (no additional changes in PR).

@vrozov, I didn't correctly express my point of view: I don't see a reason for using methods from third-party libraries if JDK provides methods with exactly the same functionality. Such usages of third-party libraries should be prevented for the consistency.

Using methods from JDK is also helps to prevent problems with updating library versions, since the API of a third-party library may change, but JDK guarantees backward compatibility.

@vvysotskyi I already explained reasons to keep Preconditions in my other comments. Here is the summary in random order:

The change does not remove the dependency on the guava library.

Preconditions are not deprecated by the guava team and provides a stable interface.

Preconditions functionality is much wider compared to JDK classes.

Using Preconditions will be more consistent with other checks like checkState or checkArgument and also with other Apache projects.

It avoids unnecessary changes and keeps original contributions.

@vvysotskyi arguments of both of you are really reasonable. But I prefer don't make changes without obvious benefit. If you insist, please describe the case and post it as [DISCUSSION] in Drill dev mailing list with a link to current PR.

I agree with what @vrozov said; technically, there seems to be no clear benefit of replacing existing Preconditions. Personal preference should only apply to new contributions , not to old contributions (since the original contributor(s) could also have her/his preference).

Thank you all for discussing this. I have reverted my changes where I replaced Preconditions.checkNotNull() with Objects.requireNonNull()

vdiravka · 2018-08-02T15:28:00Z

contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/MapRDBGroupScan.java

@@ -42,9 +44,6 @@
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Stopwatch;


Consider using of org.apache.commons.lang3.time.StopWatch everywhere

The same as for Preconditions.checkArgument()

vdiravka · 2018-08-02T16:02:04Z

.../storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseStoragePluginConfig.java

@@ -30,7 +31,6 @@
 import com.fasterxml.jackson.annotation.JsonTypeName;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableMap;


Could we replace ImmutableMap with HashMap in getConfig() method? Not sure that ImmutableMap is necessary. The method can return new HashMap(config)

I assume ImmutableMap was used here to prevent modification of the returned map. So I leave it as it is.

vdiravka · 2018-08-17T07:24:18Z

exec/java-exec/src/test/java/org/apache/drill/exec/server/rest/spnego/TestSpnegoConfig.java

@@ -106,7 +106,7 @@ public void testInvalidSpnegoConfig() throws Exception {
  @Test
  public void testSpnegoConfigOnlyKeytab() throws Exception {
    try {
-      final DrillConfig newConfig = new DrillConfig(DrillConfig.create().withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, ConfigValueFactory.fromAnyRef(true)).withValue(ExecConstants.AUTHENTICATION_MECHANISMS, ConfigValueFactory.fromIterable(Lists.newArrayList("plain"))).withValue(ExecConstants.HTTP_SPNEGO_KEYTAB, ConfigValueFactory.fromAnyRef(spnegoHelper.serverKeytab.toString())).withValue(ExecConstants.USER_AUTHENTICATOR_IMPL, ConfigValueFactory.fromAnyRef(UserAuthenticatorTestImpl.TYPE)));
+      final DrillConfig newConfig = new DrillConfig(DrillConfig.create().withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, ConfigValueFactory.fromAnyRef(true)).withValue(ExecConstants.AUTHENTICATION_MECHANISMS, ConfigValueFactory.fromIterable(Collections.singletonList("plain"))).withValue(ExecConstants.HTTP_SPNEGO_KEYTAB, ConfigValueFactory.fromAnyRef(spnegoHelper.serverKeytab.toString())).withValue(ExecConstants.USER_AUTHENTICATOR_IMPL, ConfigValueFactory.fromAnyRef(UserAuthenticatorTestImpl.TYPE)));


vdiravka · 2018-08-17T07:24:40Z

exec/java-exec/src/test/java/org/apache/drill/exec/server/rest/spnego/TestSpnegoConfig.java

@@ -123,7 +123,7 @@ public void testSpnegoConfigOnlyKeytab() throws Exception {
  @Test
  public void testSpnegoConfigOnlyPrincipal() throws Exception {
    try {
-      final DrillConfig newConfig = new DrillConfig(DrillConfig.create().withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, ConfigValueFactory.fromAnyRef(true)).withValue(ExecConstants.AUTHENTICATION_MECHANISMS, ConfigValueFactory.fromIterable(Lists.newArrayList("plain"))).withValue(ExecConstants.HTTP_SPNEGO_PRINCIPAL, ConfigValueFactory.fromAnyRef(spnegoHelper.SERVER_PRINCIPAL)).withValue(ExecConstants.USER_AUTHENTICATOR_IMPL, ConfigValueFactory.fromAnyRef(UserAuthenticatorTestImpl.TYPE)));
+      final DrillConfig newConfig = new DrillConfig(DrillConfig.create().withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, ConfigValueFactory.fromAnyRef(true)).withValue(ExecConstants.AUTHENTICATION_MECHANISMS, ConfigValueFactory.fromIterable(Collections.singletonList("plain"))).withValue(ExecConstants.HTTP_SPNEGO_PRINCIPAL, ConfigValueFactory.fromAnyRef(spnegoHelper.SERVER_PRINCIPAL)).withValue(ExecConstants.USER_AUTHENTICATOR_IMPL, ConfigValueFactory.fromAnyRef(UserAuthenticatorTestImpl.TYPE)));


vdiravka · 2018-08-17T07:30:26Z

exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillCursor.java

@@ -55,9 +55,6 @@
 import org.apache.drill.jdbc.SqlTimeoutException;
 import org.slf4j.Logger;

-import com.google.common.collect.Queues;
-
-
 class DrillCursor implements Cursor {

  ////////////////////////////////////////


remove this crazy doc

vdiravka · 2018-08-17T07:33:23Z

.../jdbc/src/test/java/org/apache/drill/jdbc/test/Bug1735ResultSetCloseReleasesBuffersTest.java

-            } catch ( SQLException e ) {
-              throw new RuntimeException( e );
-            }
+        (Function<Connection, Void>) connection -> {


Possibly rename class name, Test should in the beginning of the name
Test annotation, proper test name

I don't think that it is essential at least for current PR.

Right, there are a bunch of other tests in this package with a similar name.

vdiravka · 2018-08-17T07:35:28Z

exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestJdbcQuery.java

+                "from cp.`employee.json` limit 1");
+
+            resultSet.next();
+            final Date date = resultSet.getDate(1);


vvysotskyi

@vdiravka, thanks for the code review, I have addressed CR comments, could you please take a look again?

vvysotskyi · 2018-08-17T08:24:08Z

common/src/main/java/org/apache/drill/common/KerberosUtil.java

@@ -57,9 +57,9 @@ public static String getPrincipalFromParts(final String primary, final String in
  public static String[] splitPrincipalIntoParts(final String principal) {
    final String[] components = principal.split("[/@]");
    checkState(components.length == 3);


I think we should leave it as it is since the goal of this PR is to replace guava usage by JDK where it is possible, but there is no sense to replace usage of one library by another.

vvysotskyi · 2018-08-17T09:02:16Z

common/src/main/java/org/apache/drill/common/collections/MapWithOrdinal.java

-        }
-      }));
+      return primary.entrySet().stream()
+          .map((Function<Entry<K, Entry<Integer, V>>, Entry<K, V>>) entry ->


Thanks for pointing this, removed.

vvysotskyi · 2018-08-17T09:04:31Z

contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/MapRDBGroupScan.java

@@ -42,9 +44,6 @@
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Stopwatch;


The same as for Preconditions.checkArgument()

vvysotskyi · 2018-08-17T09:04:51Z

common/src/test/java/org/apache/drill/test/SubDirTestWatcher.java

    this.createAtBeginning = createAtBeginning;
    this.deleteAtEnd = deleteAtEnd;
-    this.subDirs = Preconditions.checkNotNull(subDirs);
+    this.subDirs = Objects.requireNonNull(subDirs);

    Preconditions.checkArgument(!subDirs.isEmpty(), "The list of subDirs is empty.");


See my previous comment

vvysotskyi · 2018-08-17T09:13:18Z

.../storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseStoragePluginConfig.java

@@ -30,7 +31,6 @@
 import com.fasterxml.jackson.annotation.JsonTypeName;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableMap;


I assume ImmutableMap was used here to prevent modification of the returned map. So I leave it as it is.

vvysotskyi · 2018-08-17T12:09:27Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java

@@ -242,7 +241,7 @@ public String getAdminUserGroups(OptionManager optionManager) {
      // if this option has not been changed by the user then return the
      // process user groups
      if (adminUserGroups.equals(DEFAULT_ADMIN_USER_GROUPS)) {
-        adminUserGroups = Joiner.on(",").join(ImpersonationUtil.getProcessUserGroupNames());
+        adminUserGroups = String.join(",", ImpersonationUtil.getProcessUserGroupNames());


If you meant a comma inside the string literal, then I doubt that space will be useful there, since the result of this method is often split by ",". So I leave it as it is.

vvysotskyi · 2018-08-17T12:14:32Z

exec/java-exec/src/main/java/org/apache/drill/exec/store/ischema/InfoSchemaFilter.java

-      return builder.toString();
+      return function + args.stream()
+          .map(ExprNode::toString)
+          .collect(Collectors.joining(",", "(", ")"));


Adding a space here may cause failures of tests with checks for the query plan. You may create a Jira for further improvement of the output, but I don't think that we should do it in the scope of this Jira.

vvysotskyi · 2018-08-17T12:16:34Z

exec/java-exec/src/test/java/org/apache/drill/exec/server/rest/spnego/TestSpnegoConfig.java

@@ -106,7 +106,7 @@ public void testInvalidSpnegoConfig() throws Exception {
  @Test
  public void testSpnegoConfigOnlyKeytab() throws Exception {
    try {
-      final DrillConfig newConfig = new DrillConfig(DrillConfig.create().withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, ConfigValueFactory.fromAnyRef(true)).withValue(ExecConstants.AUTHENTICATION_MECHANISMS, ConfigValueFactory.fromIterable(Lists.newArrayList("plain"))).withValue(ExecConstants.HTTP_SPNEGO_KEYTAB, ConfigValueFactory.fromAnyRef(spnegoHelper.serverKeytab.toString())).withValue(ExecConstants.USER_AUTHENTICATOR_IMPL, ConfigValueFactory.fromAnyRef(UserAuthenticatorTestImpl.TYPE)));
+      final DrillConfig newConfig = new DrillConfig(DrillConfig.create().withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, ConfigValueFactory.fromAnyRef(true)).withValue(ExecConstants.AUTHENTICATION_MECHANISMS, ConfigValueFactory.fromIterable(Collections.singletonList("plain"))).withValue(ExecConstants.HTTP_SPNEGO_KEYTAB, ConfigValueFactory.fromAnyRef(spnegoHelper.serverKeytab.toString())).withValue(ExecConstants.USER_AUTHENTICATOR_IMPL, ConfigValueFactory.fromAnyRef(UserAuthenticatorTestImpl.TYPE)));


vvysotskyi · 2018-08-17T12:17:37Z

exec/java-exec/src/test/java/org/apache/drill/exec/server/rest/spnego/TestSpnegoConfig.java

@@ -123,7 +123,7 @@ public void testSpnegoConfigOnlyKeytab() throws Exception {
  @Test
  public void testSpnegoConfigOnlyPrincipal() throws Exception {
    try {
-      final DrillConfig newConfig = new DrillConfig(DrillConfig.create().withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, ConfigValueFactory.fromAnyRef(true)).withValue(ExecConstants.AUTHENTICATION_MECHANISMS, ConfigValueFactory.fromIterable(Lists.newArrayList("plain"))).withValue(ExecConstants.HTTP_SPNEGO_PRINCIPAL, ConfigValueFactory.fromAnyRef(spnegoHelper.SERVER_PRINCIPAL)).withValue(ExecConstants.USER_AUTHENTICATOR_IMPL, ConfigValueFactory.fromAnyRef(UserAuthenticatorTestImpl.TYPE)));
+      final DrillConfig newConfig = new DrillConfig(DrillConfig.create().withValue(ExecConstants.USER_AUTHENTICATION_ENABLED, ConfigValueFactory.fromAnyRef(true)).withValue(ExecConstants.AUTHENTICATION_MECHANISMS, ConfigValueFactory.fromIterable(Collections.singletonList("plain"))).withValue(ExecConstants.HTTP_SPNEGO_PRINCIPAL, ConfigValueFactory.fromAnyRef(spnegoHelper.SERVER_PRINCIPAL)).withValue(ExecConstants.USER_AUTHENTICATOR_IMPL, ConfigValueFactory.fromAnyRef(UserAuthenticatorTestImpl.TYPE)));


vvysotskyi · 2018-08-17T12:25:38Z

.../jdbc/src/test/java/org/apache/drill/jdbc/test/Bug1735ResultSetCloseReleasesBuffersTest.java

-            } catch ( SQLException e ) {
-              throw new RuntimeException( e );
-            }
+        (Function<Connection, Void>) connection -> {


I don't think that it is essential at least for current PR.

weijietong · 2018-08-17T13:34:11Z

A question about this PR: what's the benefit we can gain with this replacement ?

vvysotskyi · 2018-08-17T13:37:43Z

@weijietong, this PR is an intermediate step for #1264

weijietong · 2018-08-17T13:44:33Z

get it, thanks for the explanation.

vvysotskyi · 2018-08-17T17:39:25Z

Besides the commit were addressed CR comments, I have added two commits: the first one contains changes in commits before the rebase (some of the commits added methods which I tried to replace, so replaced them again).
The second commit contains changes to ban some guava methods.

vdiravka

@vvysotskyi Thanks for adding signatures.txt, it will be useful to avoid further usage of deprecated Guava methods. The changes looks good for me, I have left some minor comments, please consider them.

vdiravka · 2018-08-20T09:42:26Z

.../jdbc/src/test/java/org/apache/drill/jdbc/test/Bug1735ResultSetCloseReleasesBuffersTest.java

-            } catch ( SQLException e ) {
-              throw new RuntimeException( e );
-            }
+        (Function<Connection, Void>) connection -> {


Right, there are a bunch of other tests in this package with a similar name.

vdiravka · 2018-08-20T09:50:15Z

common/src/main/java/org/apache/drill/common/KerberosUtil.java

@@ -57,9 +57,9 @@ public static String getPrincipalFromParts(final String primary, final String in
  public static String[] splitPrincipalIntoParts(final String principal) {
    final String[] components = principal.split("[/@]");
    checkState(components.length == 3);


Nothing wrong with Guava Preconditions and with Apache Commons as well.
IMO the method names in Apache Commons are more clear.

But I've wrongly supposed that the purpose was to get rid Guava as much as possible.

We can leave Guava Preconditions, since it is already written and usage of it is more common.

vdiravka · 2018-08-20T10:28:09Z

common/src/test/java/org/apache/drill/test/SubDirTestWatcher.java

    this.createAtBeginning = createAtBeginning;
    this.deleteAtEnd = deleteAtEnd;
-    this.subDirs = Preconditions.checkNotNull(subDirs);
+    this.subDirs = Objects.requireNonNull(subDirs);

    Preconditions.checkArgument(!subDirs.isEmpty(), "The list of subDirs is empty.");


Objects.requireNonNull is introduced mostly for Streams, but not for replacing Guava Preconditions.
From other hand they have the same implementation. Except Preconditions.checkNotNull() overloaded method with NPE error message formatting and Guava recommends to use Preconditions, see Guava master.

Looks like Calcite project replaces these methods, see Calcite master, but Apex doesn't replace them.

Since we stay with Guava it could be reasonably to follow their documentation.
But it is not critical, so I will leave the decision for you.

vdiravka · 2018-08-20T10:28:43Z

contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/MapRDBGroupScan.java

@@ -42,9 +44,6 @@
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Stopwatch;


vdiravka · 2018-08-20T10:46:42Z

.../storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseStoragePluginConfig.java

@@ -30,7 +31,6 @@
 import com.fasterxml.jackson.annotation.JsonTypeName;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableMap;


vdiravka · 2018-08-20T12:06:53Z

exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/ZookeeperClient.java

          version.setVersion(stat.getVersion());
          return bytes;
        }
        return curator.getData().forPath(target);
-      } catch (final Exception ex) {
+      } catch (Exception ex) {


ex is used rarelya and more widespread in other languages.
But ok, you can leave it.

vdiravka · 2018-08-20T12:18:11Z

exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/ExternalSortBatch.java

    currSpillDirs.add(currSpillPath);
-    String outputFile = Joiner.on("/").join(currSpillPath, spillCount++);
+    String outputFile = currSpillPath+ "/" + spillCount++;


String.valueOf(), but the statement will be longer, ok, let's leave it as is.

vdiravka · 2018-08-20T12:26:07Z

exec/java-exec/src/main/java/org/apache/drill/exec/schema/ObjectSchema.java

+                removedFields.add(field);
+                return true;
+            } else if (field.hasSchema()) {
+                Iterables.addAll(removedFields, field.getAssignedSchema().removeUnreadFields());


It makes sense.

vdiravka · 2018-08-20T12:28:28Z

exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java

@@ -242,7 +241,7 @@ public String getAdminUserGroups(OptionManager optionManager) {
      // if this option has not been changed by the user then return the
      // process user groups
      if (adminUserGroups.equals(DEFAULT_ADMIN_USER_GROUPS)) {
-        adminUserGroups = Joiner.on(",").join(ImpersonationUtil.getProcessUserGroupNames());
+        adminUserGroups = String.join(",", ImpersonationUtil.getProcessUserGroupNames());


It depends on where adminUserGroups is used.
Ok, let's leave it as is

vdiravka · 2018-08-20T12:30:16Z

exec/java-exec/src/main/java/org/apache/drill/exec/store/ischema/InfoSchemaFilter.java

-      return builder.toString();
+      return function + args.stream()
+          .map(ExprNode::toString)
+          .collect(Collectors.joining(",", "(", ")"));


Not major issue. Let's leave it as is

vvysotskyi

@vdiravka, thanks for reviewing all the changes, I have replaced usage of ImmutableMap.of() and ImmutableSet.of(), could you please take a look?

vvysotskyi · 2018-08-20T14:19:51Z

common/src/test/java/org/apache/drill/test/SubDirTestWatcher.java

    this.createAtBeginning = createAtBeginning;
    this.deleteAtEnd = deleteAtEnd;
-    this.subDirs = Preconditions.checkNotNull(subDirs);
+    this.subDirs = Objects.requireNonNull(subDirs);

    Preconditions.checkArgument(!subDirs.isEmpty(), "The list of subDirs is empty.");


I prefer using JDK things instead of third-party libraries, so I leave code with Objects.requireNonNull() (no additional changes in PR).

vvysotskyi · 2018-08-20T15:30:57Z

contrib/storage-hive/core/src/test/java/org/apache/drill/exec/fn/hive/TestInbuiltHiveUDFs.java

@@ -62,16 +62,16 @@ public void testEncode() throws Exception {

  @Test
  public void testXpath_Double() throws Exception {
-    final String query = "select xpath_double ('<a><b>20</b><c>40</c></a>', 'a/b * a/c') as col \n" +
+    String query = "select xpath_double ('<a><b>20</b><c>40</c></a>', 'a/b * a/c') as col \n" +
        "from hive.kv \n" +


I agree with you that the purpose of the test is not directly connected with limit 0, but it helps to highlight that the schema only is required.
Thanks for providing me with a chance to make the final decision :) I'll leave the code as it is.

vvysotskyi · 2018-08-20T15:31:58Z

.../storage-mongo/src/test/java/org/apache/drill/exec/store/mongo/TestMongoChunkAssignment.java

@@ -162,7 +163,7 @@ public void setUp() throws UnknownHostException {
  @Test
  public void testMongoGroupScanAssignmentMix() throws UnknownHostException,
      ExecutionSetupException {
-    final List<DrillbitEndpoint> endpoints = Lists.newArrayList();
+    final List<DrillbitEndpoint> endpoints = new ArrayList<>();


vdiravka

+1 LGTM

vrozov

There is no need to change Preconditions.checkNotNull to Objects.requireNonNull.

…requireNonNull

vvysotskyi

I have reverted my changes connected with Preconditions.checkNotNull().
@vdiravka, @vrozov, could you please take a look again?

vvysotskyi · 2018-08-23T15:24:56Z

common/src/test/java/org/apache/drill/test/SubDirTestWatcher.java

    this.createAtBeginning = createAtBeginning;
    this.deleteAtEnd = deleteAtEnd;
-    this.subDirs = Preconditions.checkNotNull(subDirs);
+    this.subDirs = Objects.requireNonNull(subDirs);

    Preconditions.checkArgument(!subDirs.isEmpty(), "The list of subDirs is empty.");


Thank you all for discussing this. I have reverted my changes where I replaced Preconditions.checkNotNull() with Objects.requireNonNull()

vrozov · 2018-08-23T16:43:47Z

common/src/main/java/org/apache/drill/common/collections/ImmutableEntry.java

 import com.google.common.base.Preconditions;

+import java.util.Map;
+import java.util.Objects;
+
 public class ImmutableEntry<K, V> implements Map.Entry<K, V>  {


@vvysotskyi In case you want to replace all Guava classes with JDK ones, why not first to replace Drill custom classes with the JDK equivalent?

Good question. What about org.apache.drill.common.collections.Collectors? You have introduced it, when Drill has already used jdk8. Is it safe to replace this class with java.util.stream.Collectors or some methods only?

The idea of replacing Guava classes appeared during the work on #1264 and changes became larger than it was expected initially.

The idea of replacing Drill custom classes is good, but it beyond the scope of the current PR, so I think it would be better to address it in a separate Jira.

@vdiravka The purpose of org.apache.drill.common.collections.Collectors is to use internal iterator with a functional interface instead of creating a stream. In many cases, I'd recommend to use Collectors and extend this class. Using streams is justified when processing thousands of elements, for a handful number of elements, Collectors is a better option, IMO.

@vvysotskyi Personally, I don't see why it is necessary to replace all Guava classes/methods with provided by JDK. IMO, the scope of PR should be to replace only removed and deprecated by Guava team API, so it can be reduced significantly.

I don't see why it is necessary to change ImmutableEntry class as part of this PR when it needs to be completely replaced with the one provided by JDK.

@vrozov sounds reasonable. Thank you for the explanation.
Does it make sense to add this info to the JavaDoc for this class and to rename this class to avoid collision with jdk Collectors?

@vdiravka Yes, please feel free to add any useful information that may be missing to java doc of the Collectors class and/or add missing functionality. I don't think that rename is necessary (use either one).

vrozov · 2018-08-23T19:49:56Z

common/src/main/java/org/apache/drill/common/collections/MapWithOrdinal.java

@@ -54,7 +51,7 @@
 public class MapWithOrdinal<K, V> implements Map<K, V> {
  private final static Logger logger = LoggerFactory.getLogger(MapWithOrdinal.class);

-  private final Map<K, Entry<Integer, V>> primary = Maps.newLinkedHashMap();
+  private final Map<K, Entry<Integer, V>> primary = new LinkedHashMap<>();
  private final IntObjectHashMap<V> secondary = new IntObjectHashMap<>();

  private final Map<K, V> delegate = new Map<K, V>() {


@vvysotskyi The same question as in #1264 what is the purpose (use case) of the class?

arina-ielchiieva · 2018-08-24T12:49:10Z

@vrozov / @vdiravka this PR has been open for a month already. I think you all agree that Volodymyr has done enormous work, doing replacement is not the most interesting or rewarding part for the developer but Volodymyr did that, also he has already made changes after several rounds of code review. So let's not demotivate him unless you want to rework the PR yourself.
Please wrap up your comments (rather than adding new ones each day), highlight what should be done to get it merged. We cannot fix all Drill problems in one PR, so please agree on follow up Jira when possible. Let's target to merge the PR next week.

vrozov · 2018-08-25T15:41:32Z

@arina-ielchiieva / @vvysotskyi I don't see a benefit of blindly replacing Guava classes/methods with JDK ones that provide equivalent or similar functionality. It should be sufficient to replace deprecated and unstable classes/methods. So, even though an enormous amount of work was done, I don't see how Drill and drill dev community benefit from that work. I already mentioned that in my review comments and also on the mailing list. IMO, it will be more beneficial for Drill customers to resolve DRILL-6422 and #1264.

@arina-ielchiieva:

I don't share your concern with PR being open for a month. In Apache PR may be open for much longer and there are multiple examples of PRs being open for 2, 3 and more month in Drill and other Apache projects.
I agree that it is not only possible but it is much better not to fix all problems in one PR. In some cases, instead of a follow up JIRA that may never be fixed, I'd prefer to see a pre-cursor JIRA and PR and this PR is an example where I'd like to see several Drill classes being removed or refactored first (due to a larger benefit that I see for removing those classes compared to removing Guava classes).

vvysotskyi · 2018-08-27T15:44:58Z

The main aim for replacing usage of guava was code consistency. Except for methods, deprecated by guava team, there was a lot of other classes/methods, which without any risks may be replaced with JDK analogs. Among them a lot of functional interfaces (Supplier, Function etc.), utility methods etc.

This pull request took much more time and effort than it should, comparing to its benefit, so I don't want to work on it anymore.

vdiravka requested changes Aug 17, 2018

View reviewed changes

vvysotskyi commented Aug 17, 2018

View reviewed changes

vvysotskyi force-pushed the DRILL-6633 branch from 71fbd03 to bae7b2e Compare August 17, 2018 13:00

vdiravka reviewed Aug 20, 2018

View reviewed changes

vvysotskyi commented Aug 20, 2018

View reviewed changes

vvysotskyi force-pushed the DRILL-6633 branch from 242bfd2 to 69a03e3 Compare August 20, 2018 15:57

vdiravka approved these changes Aug 20, 2018

View reviewed changes

vrozov suggested changes Aug 20, 2018

View reviewed changes

vvysotskyi added 6 commits August 23, 2018 15:22

DRILL-6633: Replace usage of Guava classes by JDK ones

4d22e80

DRILL-6633: Changes after CR

3d8004d

DRILL-6633: Handle changes in commits before the rebase

b89a216

DRILL-6633: Ban some methods from Guava classes

dfe3039

DRILL-6633: Changes after the second round of CR

de88c6c

DRILL-6633: Revert replacing Preconditions.checkNotNull with Objects.…

abf252d

…requireNonNull

vvysotskyi commented Aug 23, 2018

View reviewed changes

vvysotskyi force-pushed the DRILL-6633 branch from 69a03e3 to abf252d Compare August 23, 2018 15:27

vrozov reviewed Aug 23, 2018

View reviewed changes

vvysotskyi closed this Aug 27, 2018

DRILL-6633: Replace usage of Guava classes by JDK ones #1397

DRILL-6633: Replace usage of Guava classes by JDK ones #1397

Conversation

vvysotskyi commented Jul 25, 2018

arina-ielchiieva commented Jul 25, 2018

kkhatua commented Jul 25, 2018

vvysotskyi commented Jul 26, 2018 • edited

vdiravka 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

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

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

vvysotskyi 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

weijietong commented Aug 17, 2018

vvysotskyi commented Aug 17, 2018

weijietong commented Aug 17, 2018

vvysotskyi commented Aug 17, 2018

vdiravka 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

vvysotskyi 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

vdiravka left a comment

Choose a reason for hiding this comment

vrozov left a comment

Choose a reason for hiding this comment

vvysotskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvysotskyi commented Jul 26, 2018 •

edited

vdiravka Aug 23, 2018 •

edited