[GitHub] [drill] paul-rogers opened a new pull request #2074: DRILL-7728: SPI framework

[GitBox]

paul-rogers opened a new pull request #2074:
URL: https://github.com/apache/drill/pull/2074


   # [DRILL-7728](https://issues.apache.org/jira/browse/DRILL-7728): SPI 
framework
   
   ## Description
   
   We have discussed moving Drill to a "Service Provider Interface" (SPI) model 
similar to what Presto uses, and modelled after the Java 
[ServiceLoader](https://docs.oracle.com/javase/8/docs/api/java/util/ServiceLoader.html)
 concept.
   
   There are several parts. This PR is for the core mechanism explained 
[here](https://github.com/paul-rogers/drill/blob/03b36378c6380e6cda76328f5d77bb1d298fe65d/common/src/main/java/org/apache/drill/extn/README.md)
 to host extensions in Drill. The layout seems the simplest possible way to 
handle complex plugins; comments and suggestions welcome. In particular, is 
there some existing model we can adopt? I didn't find any and the Java 
`ServiceLoader` is just a bit *too* simple.
   
   The eventual goal is to provide class loader isolation so that an extension 
can use one version of Guava, say, and Drill another. This PR implements a 
lesser form: classes from one extension will not clash with those from another. 
However, it uses the default class lookup: Drill's own classes are resolved 
first, then the extension's classes. This interim solution works fine except 
when we have a conflict. We can add the stricter form later.
   
   Some effort went into ensuring this approach will work with Java 9 (and 
later) modules, but have not done extensive testing.
   
   Once the core mechanism is available, the next step is to define the SPI 
interfaces for things like UDFs, storage plugins and format plugins.
   
   ## Documentation
   
   The file linked above will eventually be the basis for user documentation 
for how to build and install extensions using SPI.
   
   ## Testing
   
   Added unit tests. Doing so required the use of test jar files. After much 
gnashing of teeth, was able to get Maven to build them and *not* install them 
in the local repo.
   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[jira] [Created] (DRILL-7728) Drill SPI framework

[Paul Rogers (Jira)]

Paul Rogers created DRILL-7728:
--

 Summary: Drill SPI framework
 Key: DRILL-7728
 URL: https://issues.apache.org/jira/browse/DRILL-7728
 Project: Apache Drill
  Issue Type: Improvement
Affects Versions: 1.18.0
Reporter: Paul Rogers
Assignee: Paul Rogers
 Fix For: 1.18.0


Provide the basic framework to load an extension in Drill, modelled after the 
Java Service Provider concept. Excludes full class loader isolation for now.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[GitHub] [drill] paul-rogers commented on a change in pull request #2072: DRILL-7724: Refactor metadata controller batch

[GitBox]

paul-rogers commented on a change in pull request #2072:
URL: https://github.com/apache/drill/pull/2072#discussion_r418754627



##
File path: 
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/metadata/MetadataControllerBatch.java
##
@@ -127,113 +126,93 @@ protected MetadataControllerBatch(MetadataControllerPOP 
popConfig,
 ? null
 : popConfig.getContext().metadataToHandle().stream()
 .collect(Collectors.toMap(MetadataInfo::identifier, 
Function.identity()));
-this.metadataUnits = new ArrayList<>();
-this.statisticsCollector = new StatisticsCollectorImpl();
 this.columnNamesOptions = new ColumnNamesOptions(context.getOptions());
   }
 
-  protected boolean setupNewSchema() {
-container.clear();
-container.addOrGet(MetastoreAnalyzeConstants.OK_FIELD_NAME, 
Types.required(TypeProtos.MinorType.BIT), null);
-container.addOrGet(MetastoreAnalyzeConstants.SUMMARY_FIELD_NAME, 
Types.required(TypeProtos.MinorType.VARCHAR), null);
-container.buildSchema(BatchSchema.SelectionVectorMode.NONE);
-container.setEmpty();
-return true;
-  }
-
   @Override
   public IterOutcome innerNext() {
-IterOutcome outcome;
-boolean finishedLeft;
-if (finished) {
-  return IterOutcome.NONE;
-}
+while (state != State.FINISHED) {
+  switch (state) {
+case RIGHT: {
 
-if (!finishedRight) {
-  outcome = handleRightIncoming();
-  if (outcome != null) {
-return outcome;
+  // Can only return NOT_YET
+  IterOutcome outcome = handleRightIncoming();
+  if (outcome != null) {
+return outcome;
+  }
+  break;
+}
+case LEFT: {
+
+  // Can only return NOT_YET
+  IterOutcome outcome = handleLeftIncoming();
+  if (outcome != null) {
+return outcome;
+  }
+  break;
+}
+case WRITE:
+  writeToMetastore();
+  createSummary();
+  state = State.FINISHED;
+  return IterOutcome.OK_NEW_SCHEMA;
+
+case FINISHED:
+  break;
+
+default:
+  throw new IllegalStateException(state.name());
   }
 }
+return IterOutcome.NONE;
+  }
 
+  private IterOutcome handleRightIncoming() {
 outer:
-while (true) {
-  outcome = next(0, left);
+for (;;) {

Review comment:
   @vvysotskyi, thanks for the review. The `for (;;)` is a bit more terse; 
happy to put it back if doing so is clearer.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [drill] vvysotskyi commented on a change in pull request #2071: DRILL-7727 Fix protobuf warning message

[GitBox]

vvysotskyi commented on a change in pull request #2071:
URL: https://github.com/apache/drill/pull/2071#discussion_r418734189



##
File path: contrib/native/client/CMakeLists.txt
##
@@ -25,6 +25,13 @@ cmake_policy(SET CMP0043 NEW)
 cmake_policy(SET CMP0048 NEW)
 enable_testing()
 
+#
+# required version for dependencies
+#
+set (BOOST_MINIMUM_VERSION 1.54.0)
+set (PROTOBUF_MINIMUM_VERSION 3.6.1)

Review comment:
   Here is an example of the issue when another version was used for 
generating protobuf files: 
https://github.com/apache/drill/pull/2000#issuecomment-592024872. Your change 
would significantly simplify finding the root cause of such issues.
   
   By the way, here are changes in protobuf files for updating to 3.11.1 
version: 
https://github.com/apache/drill/commit/7e6fc81ef414cdeca8120c20e12fcce21893f7bf#diff-b5a38f368686e0d3202ec29cd8276d6f
   
   So it may be possible that these newer changes use some new methods 
introduced after 3.6.
   
   Regarding validating windows build, I agree that it can be done separately.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [drill] laurentgo commented on a change in pull request #2071: DRILL-7727 Fix protobuf warning message

[GitBox]

laurentgo commented on a change in pull request #2071:
URL: https://github.com/apache/drill/pull/2071#discussion_r418728429



##
File path: contrib/native/client/CMakeLists.txt
##
@@ -25,6 +25,13 @@ cmake_policy(SET CMP0043 NEW)
 cmake_policy(SET CMP0048 NEW)
 enable_testing()
 
+#
+# required version for dependencies
+#
+set (BOOST_MINIMUM_VERSION 1.54.0)
+set (PROTOBUF_MINIMUM_VERSION 3.6.1)

Review comment:
   I don't think this is quite true on C++ code, files can be regenerated 
with whatever version is present on the system (it's even explained on 
documentation how to do it). Apart from bugfixes, I don't think there are 
incompatibilities introduced between protobuf minor versions.
   
   Currently I cannot validate windows build. If someone can do it quickly, I 
can sure change it, otherwise I'd prefer to get it addressed separately.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [drill] laurentgo commented on a change in pull request #2071: DRILL-7727 Fix protobuf warning message

[GitBox]

laurentgo commented on a change in pull request #2071:
URL: https://github.com/apache/drill/pull/2071#discussion_r418728429



##
File path: contrib/native/client/CMakeLists.txt
##
@@ -25,6 +25,13 @@ cmake_policy(SET CMP0043 NEW)
 cmake_policy(SET CMP0048 NEW)
 enable_testing()
 
+#
+# required version for dependencies
+#
+set (BOOST_MINIMUM_VERSION 1.54.0)
+set (PROTOBUF_MINIMUM_VERSION 3.6.1)

Review comment:
   I don't think this is quite true on C++ code, files can be regenerated 
with whatever version is present on the system (it's even explained on how to 
do it). Apart from bugfixes, I don't think there are incompatibilities 
introduced between protobuf minor versions.
   
   Currently I cannot validate windows build. If someone can do it quickly, I 
can sure change it, otherwise I'd prefer to get it addressed separately.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [drill] vvysotskyi commented on a change in pull request #2071: DRILL-7727 Fix protobuf warning message

[GitBox]

vvysotskyi commented on a change in pull request #2071:
URL: https://github.com/apache/drill/pull/2071#discussion_r418724733



##
File path: contrib/native/client/CMakeLists.txt
##
@@ -25,6 +25,13 @@ cmake_policy(SET CMP0043 NEW)
 cmake_policy(SET CMP0048 NEW)
 enable_testing()
 
+#
+# required version for dependencies
+#
+set (BOOST_MINIMUM_VERSION 1.54.0)
+set (PROTOBUF_MINIMUM_VERSION 3.6.1)

Review comment:
   It may cause issues for other places since protobuf files were 
regenerated using 3.11 version. Looks like updating `readme.win.txt` 
instruction was missed and should be fixed to be up-to-date.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[REGRESSION]: Regression in RDBMS Storage Plugin

[Charles Givre]

Hello all, 
It appears we have a regression in the RDBMS plugin in Drill 1.18.  Please take 
a look at https://issues.apache.org/jira/browse/DRILL-7698 
.  While this specific use 
case may not be the best, I'm concerned that this could affect other JDBC 
systems.
-- C

[GitHub] [drill] laurentgo commented on pull request #2070: DRILL-7726: Update boost requirement to 1.54

[GitBox]

laurentgo commented on pull request #2070:
URL: https://github.com/apache/drill/pull/2070#issuecomment-622441178


   Yes, that was my intent before merging my change, I just don't know how many 
people have been looking at the C++ code recently.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [drill] laurentgo commented on a change in pull request #2071: DRILL-7727 Fix protobuf warning message

[GitBox]

laurentgo commented on a change in pull request #2071:
URL: https://github.com/apache/drill/pull/2071#discussion_r418582565



##
File path: contrib/native/client/CMakeLists.txt
##
@@ -25,6 +25,13 @@ cmake_policy(SET CMP0043 NEW)
 cmake_policy(SET CMP0048 NEW)
 enable_testing()
 
+#
+# required version for dependencies
+#
+set (BOOST_MINIMUM_VERSION 1.54.0)
+set (PROTOBUF_MINIMUM_VERSION 3.6.1)

Review comment:
   `readme.win.txt` mentions 3.6.1, which is why I chose that version. I 
don't think it matters that much, I just wanted to make sure `ByteSizeLong()` 
method was present.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [drill] cgivre commented on pull request #1888: DRILL-5956: Add Storage Plugin for Apache Druid

[GitBox]

cgivre commented on pull request #1888:
URL: https://github.com/apache/drill/pull/1888#issuecomment-622419667


   @akkapur 
   Hi there.  I'd agree that the protobufs are a pain.  I was having the same 
issue with a PR that I just submitted as well. 
(https://github.com/apache/drill/pull/2067).  If you're developing on a Mac, 
this tutorial helped me:  
http://google.github.io/proto-lens/installing-protoc.html
   
   But for now, I'd say don't worry about the protobufs.  Let's get the code 
reviewed and once we're ready to commit we can fix the protobufs. 



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [drill] akkapur commented on pull request #1888: DRILL-5956: Add Storage Plugin for Apache Druid

[GitBox]

akkapur commented on pull request #1888:
URL: https://github.com/apache/drill/pull/1888#issuecomment-622417910


   @cgivre hi Charles, I am having trouble getting the right version of 
protobuf to generate the files. Is this something you can help with? Currently, 
the build seems to be failing because of that.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [drill] vvysotskyi commented on a change in pull request #2071: DRILL-7727 Fix protobuf warning message

[GitBox]

vvysotskyi commented on a change in pull request #2071:
URL: https://github.com/apache/drill/pull/2071#discussion_r418484316



##
File path: contrib/native/client/CMakeLists.txt
##
@@ -25,6 +25,13 @@ cmake_policy(SET CMP0043 NEW)
 cmake_policy(SET CMP0048 NEW)
 enable_testing()
 
+#
+# required version for dependencies
+#
+set (BOOST_MINIMUM_VERSION 1.54.0)
+set (PROTOBUF_MINIMUM_VERSION 3.6.1)

Review comment:
   The current version uses protobuf 3.11 version (see readme.linux).





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [drill] vvysotskyi commented on a change in pull request #2072: DRILL-7724: Refactor metadata controller batch

[GitBox]

vvysotskyi commented on a change in pull request #2072:
URL: https://github.com/apache/drill/pull/2072#discussion_r418479001



##
File path: 
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/metadata/MetadataControllerBatch.java
##
@@ -127,113 +126,93 @@ protected MetadataControllerBatch(MetadataControllerPOP 
popConfig,
 ? null
 : popConfig.getContext().metadataToHandle().stream()
 .collect(Collectors.toMap(MetadataInfo::identifier, 
Function.identity()));
-this.metadataUnits = new ArrayList<>();
-this.statisticsCollector = new StatisticsCollectorImpl();
 this.columnNamesOptions = new ColumnNamesOptions(context.getOptions());
   }
 
-  protected boolean setupNewSchema() {
-container.clear();
-container.addOrGet(MetastoreAnalyzeConstants.OK_FIELD_NAME, 
Types.required(TypeProtos.MinorType.BIT), null);
-container.addOrGet(MetastoreAnalyzeConstants.SUMMARY_FIELD_NAME, 
Types.required(TypeProtos.MinorType.VARCHAR), null);
-container.buildSchema(BatchSchema.SelectionVectorMode.NONE);
-container.setEmpty();
-return true;
-  }
-
   @Override
   public IterOutcome innerNext() {
-IterOutcome outcome;
-boolean finishedLeft;
-if (finished) {
-  return IterOutcome.NONE;
-}
+while (state != State.FINISHED) {
+  switch (state) {
+case RIGHT: {
 
-if (!finishedRight) {
-  outcome = handleRightIncoming();
-  if (outcome != null) {
-return outcome;
+  // Can only return NOT_YET
+  IterOutcome outcome = handleRightIncoming();
+  if (outcome != null) {
+return outcome;
+  }
+  break;
+}
+case LEFT: {
+
+  // Can only return NOT_YET
+  IterOutcome outcome = handleLeftIncoming();
+  if (outcome != null) {
+return outcome;
+  }
+  break;
+}
+case WRITE:
+  writeToMetastore();
+  createSummary();
+  state = State.FINISHED;
+  return IterOutcome.OK_NEW_SCHEMA;
+
+case FINISHED:
+  break;
+
+default:
+  throw new IllegalStateException(state.name());
   }
 }
+return IterOutcome.NONE;
+  }
 
+  private IterOutcome handleRightIncoming() {
 outer:
-while (true) {
-  outcome = next(0, left);
+for (;;) {

Review comment:
   Are there any reasons for replacing `while (true)` with `for (;;)`?





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org