[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[pvillard31]

Github user pvillard31 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r21538
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive3-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHive3QLProcessor.java
 ---
@@ -345,4 +348,22 @@ private void findTableNames(final Object obj, final 
Set tableNames) {
 }
 return attributes;
 }
+
+/**
+ * Method to set the configured timeout on the statement to be executed
+ * @param stmt statement to be executed
+ * @param context process context to retrieve the configured value
+ * @param flowFile flow file to evaluate expression language
+ * @throws ProcessException exception in case configured value cannot 
be converted to an integer
+ */
+protected void setTimeout(Statement stmt, ProcessContext context, 
FlowFile flowFile) throws ProcessException {
--- End diff --

The try/catch is still valid in case of error during the EL evaluation 
(when EL is used to set the timeout value). I could remove the setTimeout in 
the abstract class and just use setQueryTimeout directly in the Select/Put 
classes but I'd still have to define how I deal with SQLException and 
NumberFormatException. Honestly I think it's better to keep it as-is to have 
some sort of consistency between the two bundles.


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r213318454
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive3-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHive3QLProcessor.java
 ---
@@ -345,4 +348,22 @@ private void findTableNames(final Object obj, final 
Set tableNames) {
 }
 return attributes;
 }
+
+/**
+ * Method to set the configured timeout on the statement to be executed
+ * @param stmt statement to be executed
+ * @param context process context to retrieve the configured value
+ * @param flowFile flow file to evaluate expression language
+ * @throws ProcessException exception in case configured value cannot 
be converted to an integer
+ */
+protected void setTimeout(Statement stmt, ProcessContext context, 
FlowFile flowFile) throws ProcessException {
--- End diff --

I mentioned it only for consistency between the two sets of processors, 
actually I'd be fine with not even putting the setQueryTimeout in a try/catch 
for the Hive 3 processors, now that it's supported, I don't imagine there are 
any drivers that wouldn't support it at this point.
Either way you want to go is fine with me, let me know what you end up with 
(even if it is what you already have :) and I'll finish the review and merge, 
thanks!


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[pvillard31]

Github user pvillard31 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r213204485
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive3-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHive3QLProcessor.java
 ---
@@ -345,4 +348,22 @@ private void findTableNames(final Object obj, final 
Set tableNames) {
 }
 return attributes;
 }
+
+/**
+ * Method to set the configured timeout on the statement to be executed
+ * @param stmt statement to be executed
+ * @param context process context to retrieve the configured value
+ * @param flowFile flow file to evaluate expression language
+ * @throws ProcessException exception in case configured value cannot 
be converted to an integer
+ */
+protected void setTimeout(Statement stmt, ProcessContext context, 
FlowFile flowFile) throws ProcessException {
--- End diff --

Yeah... decided not to because I figured that all the drivers on Hive3 and 
beyond would implement the method now. I thought it would make less validation 
work when the framework is validating the processor. But I guess we could 
choose to be on the safest side and add it anyway. Thoughts?


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r213045384
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive3-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHive3QLProcessor.java
 ---
@@ -345,4 +348,22 @@ private void findTableNames(final Object obj, final 
Set tableNames) {
 }
 return attributes;
 }
+
+/**
+ * Method to set the configured timeout on the statement to be executed
+ * @param stmt statement to be executed
+ * @param context process context to retrieve the configured value
+ * @param flowFile flow file to evaluate expression language
+ * @throws ProcessException exception in case configured value cannot 
be converted to an integer
+ */
+protected void setTimeout(Statement stmt, ProcessContext context, 
FlowFile flowFile) throws ProcessException {
--- End diff --

Should we have a customValidate() for this like you added for 
AbstractHiveQLProcessor?


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r206380004
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHiveQLProcessor.java
 ---
@@ -75,6 +81,38 @@
 .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
 .build();
 
+public static final PropertyDescriptor QUERY_TIMEOUT = new 
PropertyDescriptor.Builder()
+.name("hive-query-timeout")
+.displayName("Query timeout")
+.description("Sets the number of seconds the driver will wait 
for a query to execute. "
++ "A value of 0 means no timeout. NOTE: Non-zero 
values may not be supported by the driver.")
+.defaultValue("0")
+.required(true)
+.addValidator(StandardValidators.INTEGER_VALIDATOR)
+.expressionLanguageSupported(true)
+.build();
+
+@Override
+protected Collection 
customValidate(ValidationContext validationContext) {
+final List problems = new ArrayList<>(1);
+
+if(validationContext.getProperty(QUERY_TIMEOUT).isSet()
+&& 
!validationContext.getProperty(QUERY_TIMEOUT).isExpressionLanguagePresent()
+&& 
validationContext.getProperty(QUERY_TIMEOUT).asInteger() != 0) {
+try(HiveStatement stmt = new HiveStatement(null, null, null)) {
+stmt.setQueryTimeout(0);
--- End diff --

Also I think yes, the idea is to catch the "not supported" thing during 
customValidate() where we can, but we also have to add it to the runtime 
checking because of EL


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r206336993
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHiveQLProcessor.java
 ---
@@ -75,6 +81,38 @@
 .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
 .build();
 
+public static final PropertyDescriptor QUERY_TIMEOUT = new 
PropertyDescriptor.Builder()
+.name("hive-query-timeout")
+.displayName("Query timeout")
+.description("Sets the number of seconds the driver will wait 
for a query to execute. "
++ "A value of 0 means no timeout. NOTE: Non-zero 
values may not be supported by the driver.")
+.defaultValue("0")
+.required(true)
+.addValidator(StandardValidators.INTEGER_VALIDATOR)
+.expressionLanguageSupported(true)
+.build();
+
+@Override
+protected Collection 
customValidate(ValidationContext validationContext) {
+final List problems = new ArrayList<>(1);
+
+if(validationContext.getProperty(QUERY_TIMEOUT).isSet()
+&& 
!validationContext.getProperty(QUERY_TIMEOUT).isExpressionLanguagePresent()
+&& 
validationContext.getProperty(QUERY_TIMEOUT).asInteger() != 0) {
+try(HiveStatement stmt = new HiveStatement(null, null, null)) {
+stmt.setQueryTimeout(0);
--- End diff --

Yeah anything but zero, just so it matches the logic


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[pvillard31]

Github user pvillard31 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r206335572
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHiveQLProcessor.java
 ---
@@ -75,6 +81,38 @@
 .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
 .build();
 
+public static final PropertyDescriptor QUERY_TIMEOUT = new 
PropertyDescriptor.Builder()
+.name("hive-query-timeout")
+.displayName("Query timeout")
+.description("Sets the number of seconds the driver will wait 
for a query to execute. "
++ "A value of 0 means no timeout. NOTE: Non-zero 
values may not be supported by the driver.")
+.defaultValue("0")
+.required(true)
+.addValidator(StandardValidators.INTEGER_VALIDATOR)
+.expressionLanguageSupported(true)
+.build();
+
+@Override
+protected Collection 
customValidate(ValidationContext validationContext) {
+final List problems = new ArrayList<>(1);
+
+if(validationContext.getProperty(QUERY_TIMEOUT).isSet()
+&& 
!validationContext.getProperty(QUERY_TIMEOUT).isExpressionLanguagePresent()
+&& 
validationContext.getProperty(QUERY_TIMEOUT).asInteger() != 0) {
+try(HiveStatement stmt = new HiveStatement(null, null, null)) {
+stmt.setQueryTimeout(0);
--- End diff --

> True (at least for older Apache Hive drivers), but it is a tad confusing 
to see the if statement check for non-zero then test with zero.

Do you suggest an arbitrary value like
java
stmt.setQueryTimeout(1); // just checking driver supports query timeout

?


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[pvillard31]

Github user pvillard31 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r206334896
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHiveQLProcessor.java
 ---
@@ -76,6 +86,38 @@
 .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
 .build();
 
+public static final PropertyDescriptor QUERY_TIMEOUT = new 
PropertyDescriptor.Builder()
+.name("hive-query-timeout")
+.displayName("Query timeout")
+.description("Sets the number of seconds the driver will wait 
for a query to execute. "
++ "A value of 0 means no timeout. NOTE: Non-zero 
values may not be supported by the driver.")
+.defaultValue("0")
+.required(true)
+.addValidator(StandardValidators.INTEGER_VALIDATOR)
--- End diff --

True - will fix in AbstractHiveQLProcessor and AbstractHive3QLProcessor


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[pvillard31]

Github user pvillard31 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r206334616
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHiveQLProcessor.java
 ---
@@ -75,6 +81,38 @@
 .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
 .build();
 
+public static final PropertyDescriptor QUERY_TIMEOUT = new 
PropertyDescriptor.Builder()
+.name("hive-query-timeout")
+.displayName("Query timeout")
+.description("Sets the number of seconds the driver will wait 
for a query to execute. "
++ "A value of 0 means no timeout. NOTE: Non-zero 
values may not be supported by the driver.")
+.defaultValue("0")
+.required(true)
+.addValidator(StandardValidators.INTEGER_VALIDATOR)
+.expressionLanguageSupported(true)
+.build();
+
+@Override
+protected Collection 
customValidate(ValidationContext validationContext) {
+final List problems = new ArrayList<>(1);
+
+if(validationContext.getProperty(QUERY_TIMEOUT).isSet()
+&& 
!validationContext.getProperty(QUERY_TIMEOUT).isExpressionLanguagePresent()
+&& 
validationContext.getProperty(QUERY_TIMEOUT).asInteger() != 0) {
+try(HiveStatement stmt = new HiveStatement(null, null, null)) {
+stmt.setQueryTimeout(0);
--- End diff --

Did you mean this one: 
https://github.com/apache/nifi/pull/2138#discussion_r159670913


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r204501742
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHiveQLProcessor.java
 ---
@@ -76,6 +86,38 @@
 .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
 .build();
 
+public static final PropertyDescriptor QUERY_TIMEOUT = new 
PropertyDescriptor.Builder()
+.name("hive-query-timeout")
+.displayName("Query timeout")
+.description("Sets the number of seconds the driver will wait 
for a query to execute. "
++ "A value of 0 means no timeout. NOTE: Non-zero 
values may not be supported by the driver.")
+.defaultValue("0")
+.required(true)
+.addValidator(StandardValidators.INTEGER_VALIDATOR)
--- End diff --

I missed this in the Hive 3 processors too, but shouldn't this be a 
NONNEGATIVE_INTEGER_VALIDATOR? Are there any situations in which a negative 
number has any semantics?


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r204501609
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHiveQLProcessor.java
 ---
@@ -75,6 +81,38 @@
 .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
 .build();
 
+public static final PropertyDescriptor QUERY_TIMEOUT = new 
PropertyDescriptor.Builder()
+.name("hive-query-timeout")
+.displayName("Query timeout")
+.description("Sets the number of seconds the driver will wait 
for a query to execute. "
++ "A value of 0 means no timeout. NOTE: Non-zero 
values may not be supported by the driver.")
+.defaultValue("0")
+.required(true)
+.addValidator(StandardValidators.INTEGER_VALIDATOR)
+.expressionLanguageSupported(true)
+.build();
+
+@Override
+protected Collection 
customValidate(ValidationContext validationContext) {
+final List problems = new ArrayList<>(1);
+
+if(validationContext.getProperty(QUERY_TIMEOUT).isSet()
+&& 
!validationContext.getProperty(QUERY_TIMEOUT).isExpressionLanguagePresent()
+&& 
validationContext.getProperty(QUERY_TIMEOUT).asInteger() != 0) {
+try(HiveStatement stmt = new HiveStatement(null, null, null)) {
+stmt.setQueryTimeout(0);
--- End diff --

True (at least for older Apache Hive drivers), but it is a tad confusing to 
see the if statement check for non-zero then test with zero.

Also, I can't find the discussion but I thought we were going to do similar 
error handling in the setTimeout() method below as we do in the customValidate, 
for when Expression Language is present but the driver doesn't support non-zero 
values. IIRC it would allow a query timeout of zero if the driver didn't 
support it, but if the user set it to a positive value and the driver didn't 
support it, it would throw an error (akin to being invalid if found in 
customValidate())?


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[pvillard31]

Github user pvillard31 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r168924543
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/SelectHiveQL.java
 ---
@@ -310,6 +311,15 @@ private void onTrigger(final ProcessContext context, 
final ProcessSession sessio
 try (final Connection con = dbcpService.getConnection();
  final Statement st = (flowbased ? 
con.prepareStatement(selectQuery) : con.createStatement())
 ) {
+try {
+final int queryTimeout = 
context.getProperty(QUERY_TIMEOUT).evaluateAttributeExpressions(fileToProcess).asInteger();
--- End diff --

Good point. I just pushed a commit to address it.


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[pvillard31]

Github user pvillard31 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r168924048
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHiveQLProcessor.java
 ---
@@ -75,6 +81,38 @@
 .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
 .build();
 
+public static final PropertyDescriptor QUERY_TIMEOUT = new 
PropertyDescriptor.Builder()
+.name("hive-query-timeout")
+.displayName("Query timeout")
+.description("Sets the number of seconds the driver will wait 
for a query to execute. "
++ "A value of 0 means no timeout. NOTE: Non-zero 
values may not be supported by the driver.")
+.defaultValue("0")
+.required(true)
+.addValidator(StandardValidators.INTEGER_VALIDATOR)
+.expressionLanguageSupported(true)
+.build();
+
+@Override
+protected Collection 
customValidate(ValidationContext validationContext) {
+final List problems = new ArrayList<>(1);
+
+if(validationContext.getProperty(QUERY_TIMEOUT).isSet()
+&& 
!validationContext.getProperty(QUERY_TIMEOUT).isExpressionLanguagePresent()
+&& 
validationContext.getProperty(QUERY_TIMEOUT).asInteger() != 0) {
+try(HiveStatement stmt = new HiveStatement(null, null, null)) {
+stmt.setQueryTimeout(0);
--- End diff --

Actually, in versions of the driver that does not implement this method, 
this call will throw an exception no matter what is the value.


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[MikeThomsen]

Github user MikeThomsen commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r165940928
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHiveQLProcessor.java
 ---
@@ -75,6 +81,38 @@
 .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
 .build();
 
+public static final PropertyDescriptor QUERY_TIMEOUT = new 
PropertyDescriptor.Builder()
+.name("hive-query-timeout")
+.displayName("Query timeout")
+.description("Sets the number of seconds the driver will wait 
for a query to execute. "
++ "A value of 0 means no timeout. NOTE: Non-zero 
values may not be supported by the driver.")
+.defaultValue("0")
+.required(true)
+.addValidator(StandardValidators.INTEGER_VALIDATOR)
+.expressionLanguageSupported(true)
+.build();
+
+@Override
+protected Collection 
customValidate(ValidationContext validationContext) {
+final List problems = new ArrayList<>(1);
+
+if(validationContext.getProperty(QUERY_TIMEOUT).isSet()
+&& 
!validationContext.getProperty(QUERY_TIMEOUT).isExpressionLanguagePresent()
+&& 
validationContext.getProperty(QUERY_TIMEOUT).asInteger() != 0) {
+try(HiveStatement stmt = new HiveStatement(null, null, null)) {
+stmt.setQueryTimeout(0);
--- End diff --

You say in the description:

> A value of 0 means no timeout. NOTE: Non-zero values may not be supported 
by the driver.

Shouldn't the test be on a non-zero value here?


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[MikeThomsen]

Github user MikeThomsen commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r165941527
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/SelectHiveQL.java
 ---
@@ -310,6 +311,15 @@ private void onTrigger(final ProcessContext context, 
final ProcessSession sessio
 try (final Connection con = dbcpService.getConnection();
  final Statement st = (flowbased ? 
con.prepareStatement(selectQuery) : con.createStatement())
 ) {
+try {
+final int queryTimeout = 
context.getProperty(QUERY_TIMEOUT).evaluateAttributeExpressions(fileToProcess).asInteger();
--- End diff --

This looks fungible with the logic from `PutHive`. Why not move it to the 
abstract base class as a method both can reuse?


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[pvillard31]

Github user pvillard31 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r160469685
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/PutHiveQL.java
 ---
@@ -232,6 +233,13 @@ private FunctionContext(boolean rollbackOnFailure, 
Charset charset, String state
 getLogger().warn("Failed to parse hiveQL: {} due 
to {}", new Object[]{hiveQL, e}, e);
 }
 
+try {
+// set query timeout
+
stmt.setQueryTimeout(context.getProperty(QUERY_TIMEOUT).evaluateAttributeExpressions(flowFile).asInteger());
--- End diff --

Oh right, forgot about EL! I think best is to keep current behavior with an 
exception and rollback but with another logging statement. I'll push a commit 
for that.


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r160467825
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/PutHiveQL.java
 ---
@@ -232,6 +233,13 @@ private FunctionContext(boolean rollbackOnFailure, 
Charset charset, String state
 getLogger().warn("Failed to parse hiveQL: {} due 
to {}", new Object[]{hiveQL, e}, e);
 }
 
+try {
+// set query timeout
+
stmt.setQueryTimeout(context.getProperty(QUERY_TIMEOUT).evaluateAttributeExpressions(flowFile).asInteger());
--- End diff --

No, if Expression Language is present, INTEGER_VALIDATOR will return Valid 
as it may not be able to evaluate the EL at validation time.


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[pvillard31]

Github user pvillard31 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r160465819
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/PutHiveQL.java
 ---
@@ -232,6 +233,13 @@ private FunctionContext(boolean rollbackOnFailure, 
Charset charset, String state
 getLogger().warn("Failed to parse hiveQL: {} due 
to {}", new Object[]{hiveQL, e}, e);
 }
 
+try {
+// set query timeout
+
stmt.setQueryTimeout(context.getProperty(QUERY_TIMEOUT).evaluateAttributeExpressions(flowFile).asInteger());
--- End diff --

That shouldn't happen with the defined validator on the property, no?
java
.addValidator(StandardValidators.INTEGER_VALIDATOR)



---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r160460513
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/PutHiveQL.java
 ---
@@ -232,6 +233,13 @@ private FunctionContext(boolean rollbackOnFailure, 
Charset charset, String state
 getLogger().warn("Failed to parse hiveQL: {} due 
to {}", new Object[]{hiveQL, e}, e);
 }
 
+try {
+// set query timeout
+
stmt.setQueryTimeout(context.getProperty(QUERY_TIMEOUT).evaluateAttributeExpressions(flowFile).asInteger());
--- End diff --

Unfortunately asInteger() will throw a NumberFormatException if the EL 
evaluates to a blank string/null. Should we warn that we're defaulting to zero, 
or keep the current behavior which is an exception and rollback, perhaps adding 
another logging statement to clarify the one that will follow (NFE for input 
string "") ?


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[pvillard31]

Github user pvillard31 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r160459279
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHiveQLProcessor.java
 ---
@@ -75,6 +81,38 @@
 .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
 .build();
 
+public static final PropertyDescriptor QUERY_TIMEOUT = new 
PropertyDescriptor.Builder()
+.name("hive-query-timeout")
+.displayName("Query timeout")
+.description("Sets the number of seconds the driver will wait 
for a query to execute. "
++ "A value of 0 means no timeout. NOTE: Non-zero 
values may not be supported by the driver.")
+.defaultValue("0")
+.required(true)
+.addValidator(StandardValidators.INTEGER_VALIDATOR)
+.expressionLanguageSupported(true)
+.build();
+
+@Override
+protected Collection 
customValidate(ValidationContext validationContext) {
+final List problems = new ArrayList<>(1);
+
+if(validationContext.getProperty(QUERY_TIMEOUT).isSet()
+&& 
!validationContext.getProperty(QUERY_TIMEOUT).isExpressionLanguagePresent()
+&& 
validationContext.getProperty(QUERY_TIMEOUT).asInteger() != 0) {
+try(HiveStatement stmt = new HiveStatement(null, null, null)) {
+stmt.setQueryTimeout(0);
+} catch (SQLException e) {
+problems.add(new ValidationResult.Builder()
+.subject("Query Timeout")
+.valid(false)
+.explanation(e.getLocalizedMessage())
--- End diff --

Yes sure @mattyb149 !


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r160458722
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHiveQLProcessor.java
 ---
@@ -75,6 +81,38 @@
 .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
 .build();
 
+public static final PropertyDescriptor QUERY_TIMEOUT = new 
PropertyDescriptor.Builder()
+.name("hive-query-timeout")
+.displayName("Query timeout")
+.description("Sets the number of seconds the driver will wait 
for a query to execute. "
++ "A value of 0 means no timeout. NOTE: Non-zero 
values may not be supported by the driver.")
+.defaultValue("0")
+.required(true)
+.addValidator(StandardValidators.INTEGER_VALIDATOR)
+.expressionLanguageSupported(true)
+.build();
+
+@Override
+protected Collection 
customValidate(ValidationContext validationContext) {
+final List problems = new ArrayList<>(1);
+
+if(validationContext.getProperty(QUERY_TIMEOUT).isSet()
+&& 
!validationContext.getProperty(QUERY_TIMEOUT).isExpressionLanguagePresent()
+&& 
validationContext.getProperty(QUERY_TIMEOUT).asInteger() != 0) {
+try(HiveStatement stmt = new HiveStatement(null, null, null)) {
+stmt.setQueryTimeout(0);
+} catch (SQLException e) {
+problems.add(new ValidationResult.Builder()
+.subject("Query Timeout")
+.valid(false)
+.explanation(e.getLocalizedMessage())
--- End diff --

This message is "Method not supported", I can add more text around before 
merge, such as "setQueryTimeout caused the driver to report 
"+e.getLocalizedMessage() or something like that?


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[pvillard31]

Github user pvillard31 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r159671901
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/PutHiveQL.java
 ---
@@ -233,6 +234,7 @@ private FunctionContext(boolean rollbackOnFailure, 
Charset charset, String state
 }
 
 // Execute the statement
+
stmt.setQueryTimeout(context.getProperty(QUERY_TIMEOUT).evaluateAttributeExpressions(flowFile).asInteger());
--- End diff --

Good catch, just pushed a commit for PutHiveQL and SelectHiveQL. Thanks!


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r159670913
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/PutHiveQL.java
 ---
@@ -233,6 +234,7 @@ private FunctionContext(boolean rollbackOnFailure, 
Charset charset, String state
 }
 
 // Execute the statement
+
stmt.setQueryTimeout(context.getProperty(QUERY_TIMEOUT).evaluateAttributeExpressions(flowFile).asInteger());
--- End diff --

If the driver doesn't support this (at least for Apache Hive 1.2.1), it 
will throw an exception, perhaps wrap these calls in a try/catch and ignore any 
errors (as they would have been presented to the user via doc and validation)?


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r150299869
  
--- Diff: 
nifi-mock/src/main/java/org/apache/nifi/util/MockPropertyValue.java ---
@@ -225,7 +225,7 @@ public String toString() {
 
 @Override
 public boolean isExpressionLanguagePresent() {
-if (!expectExpressions) {
+if (expectExpressions == null || !expectExpressions) {
--- End diff --

This fix (in whatever form it takes) should be as part of 
[NIFI-4590](https://issues.apache.org/jira/browse/NIFI-4590). As a temporary 
fix in my own PR #2260 , I put a try/catch around isExpressionLanguagePresent() 
with a comment about NIFI-4590. You don't have to do that specifically, but if 
you can get around changing the mock stuff in this PR, I can merge and add a 
reference to your workaround in NIFI-4590, to ask that your workaround be 
removed as part of that Jira.


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r143296927
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHiveQLProcessor.java
 ---
@@ -66,6 +71,38 @@
 .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
 .build();
 
+public static final PropertyDescriptor QUERY_TIMEOUT = new 
PropertyDescriptor.Builder()
+.name("hive-query-timeout")
+.displayName("Query timeout")
+.description("Sets the number of seconds the driver will wait 
for a query to execute. "
++ "A value of 0 means no timeout. This feature is 
available starting with Hive 2.1.")
--- End diff --

How about we replace the "This feature is available..." sentence with 
"NOTE: Non-zero values may not be supported by the driver"


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[mattyb149]

Github user mattyb149 commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r143281834
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHiveQLProcessor.java
 ---
@@ -66,6 +71,38 @@
 .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
 .build();
 
+public static final PropertyDescriptor QUERY_TIMEOUT = new 
PropertyDescriptor.Builder()
+.name("hive-query-timeout")
+.displayName("Query timeout")
+.description("Sets the number of seconds the driver will wait 
for a query to execute. "
++ "A value of 0 means no timeout. This feature is 
available starting with Hive 2.1.")
--- End diff --

The part about the feature availability is nice if/when there's a choice, 
but for now it's not technically germane, since the Hive NAR ships with a 
particular version of Hive. I will remove that part and merge, the rest LGTM :)


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[joewitt]

Github user joewitt commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r143281431
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/AbstractHiveQLProcessor.java
 ---
@@ -66,6 +71,38 @@
 .addValidator(StandardValidators.CHARACTER_SET_VALIDATOR)
 .build();
 
+public static final PropertyDescriptor QUERY_TIMEOUT = new 
PropertyDescriptor.Builder()
+.name("hive-query-timeout")
+.displayName("Query timeout")
+.description("Sets the number of seconds the driver will wait 
for a query to execute. "
++ "A value of 0 means no timeout. This feature is 
available starting with Hive 2.1.")
--- End diff --

I'd remove the 'This feature is available starting with Hive 2.1". 
Otherwise this all lgtm


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[joewitt]

Github user joewitt commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r143280686
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/SelectHiveQL.java
 ---
@@ -290,6 +292,9 @@ public void process(final OutputStream out) throws 
IOException {
 }
 }
 
+// set query timeout
+st.setQueryTimeout(queryTimeout);
--- End diff --

disregard what i said.  matt pointed out that the code checks IF a timeout 
was set and is non-zero.  Based on that I think the way it was implemented is 
awesome.


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[joewitt]

Github user joewitt commented on a diff in the pull request:

https://github.com/apache/nifi/pull/2138#discussion_r143279988
  
--- Diff: 
nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/SelectHiveQL.java
 ---
@@ -290,6 +292,9 @@ public void process(final OutputStream out) throws 
IOException {
 }
 }
 
+// set query timeout
+st.setQueryTimeout(queryTimeout);
--- End diff --

I get the point of the customValidate but I'd keep its check but mark it as 
valid whether the timeout method is supported or not.  YOu can set some 
processor instance boolean to advise whether it is supported then in this 
call/check set the timeout if it is and ignore it if not. Otherwise we're 
requiring them to use a version of hive which supports it and that seems a 
little heavy handed.  It is probably a good idea to do the current validation 
logic in some other lifecycle call like 'onAdded' or something, warn if 
timeouts not supported and point out hanging threads are possible, and keep 
going.


---

[GitHub] nifi pull request #2138: NIFI-4371 - add support for query timeout in Hive p...

[pvillard31]

GitHub user pvillard31 opened a pull request:

https://github.com/apache/nifi/pull/2138

NIFI-4371 - add support for query timeout in Hive processors

Thank you for submitting a contribution to Apache NiFi.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced 
 in the commit message?

- [ ] Does your PR title start with NIFI- where  is the JIRA number 
you are trying to resolve? Pay particular attention to the hyphen "-" character.

- [ ] Has your PR been rebased against the latest commit within the target 
branch (typically master)?

- [ ] Is your initial contribution a single, squashed commit?

### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via mvn 
-Pcontrib-check clean install at the root nifi folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies 
licensed in a way that is compatible for inclusion under [ASF 
2.0](http://www.apache.org/legal/resolved.html#category-a)? 
- [ ] If applicable, have you updated the LICENSE file, including the main 
LICENSE file under nifi-assembly?
- [ ] If applicable, have you updated the NOTICE file, including the main 
NOTICE file found under nifi-assembly?
- [ ] If adding new Properties, have you added .displayName in addition to 
.name (programmatic access) for each of the new properties?

### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in 
which it is rendered?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build 
issues and submit an update to your PR as soon as possible.


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/pvillard31/nifi NIFI-4371

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/nifi/pull/2138.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #2138


commit 8a4ec6cd7788dd83899ffd9536caa9317690134e
Author: Pierre Villard 
Date:   2017-09-09T16:57:34Z

NIFI-4371 - add support for query timeout in Hive processors




---