You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2022/03/30 17:08:32 UTC
[GitHub] [hive] zabetak opened a new pull request #3156: HIVE-26095: Add queryid in QueryLifeTimeHookContext
zabetak opened a new pull request #3156:
URL: https://github.com/apache/hive/pull/3156
### What changes were proposed in this pull request?
Add queryid in QueryLifeTimeHookContext
### Why are the changes needed?
See HIVE-26095
### Does this PR introduce _any_ user-facing change?
Yes, it introduces a new public API
### How was this patch tested?
`mvn test -Dtest=TestQueryLifeTimeHooksWithSQLOperation`
Plus existing tests cause we ensure that the queryid that is passed in the context is never null.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] amansinha100 commented on pull request #3156: HIVE-26095: Add queryid in QueryLifeTimeHookContext
Posted by GitBox <gi...@apache.org>.
amansinha100 commented on pull request #3156:
URL: https://github.com/apache/hive/pull/3156#issuecomment-1083561586
This would be very useful for diagnosability. I have one comment. Overall LGTM. +1
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kasakrisz commented on a change in pull request #3156: HIVE-26095: Add queryid in QueryLifeTimeHookContext
Posted by GitBox <gi...@apache.org>.
kasakrisz commented on a change in pull request #3156:
URL: https://github.com/apache/hive/pull/3156#discussion_r840413959
##########
File path: service/src/test/org/apache/hive/service/cli/operation/TestQueryLifeTimeHooksWithSQLOperation.java
##########
@@ -110,6 +118,45 @@ public void afterExecution(QueryLifeTimeHookContext ctx, boolean hasError) {
assertNull(ctx.getHookContext().getException());
assertNotNull(ctx.getHookContext().getQueryInfo());
assertNotNull(ctx.getHookContext().getQueryInfo().getQueryDisplay());
+ assertQueryId(ctx.getQueryId());
}
}
+
+ /**
+ * Asserts that the specified query id exists and has the expected prefix and size.
+ *
+ * <p>
+ * A query id looks like below:
+ * <pre>
+ * username_20220330093338_dab90f30-5e79-463d-8359-0d2fff57effa
+ * </pre>
+ * and we can accurately predict how the prefix should look like. T
+ * </p>
+ *
+ * @param actualQueryId the query id to verify
+ */
+ private static void assertQueryId(String actualQueryId) {
+ assertNotNull(actualQueryId);
+ String expectedIdPrefix = makeQueryIdStablePrefix();
+ String actualIdPrefix = actualQueryId.substring(0, expectedIdPrefix.length());
+ assertEquals(expectedIdPrefix, actualIdPrefix);
+ assertEquals(expectedIdPrefix.length() + 41, actualQueryId.length());
+ }
+
+ /**
+ * Makes a query id prefix that is stable for an hour. The prefix changes every hour but this is enough to guarantee
Review comment:
If getting the queryId from conf is sufficient the new API is not necessarily needed. I think sharing data via config between modules is not a nice solution but that would be a different project to get rid 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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a change in pull request #3156: HIVE-26095: Add queryid in QueryLifeTimeHookContext
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #3156:
URL: https://github.com/apache/hive/pull/3156#discussion_r840394149
##########
File path: service/src/test/org/apache/hive/service/cli/operation/TestQueryLifeTimeHooksWithSQLOperation.java
##########
@@ -110,6 +118,45 @@ public void afterExecution(QueryLifeTimeHookContext ctx, boolean hasError) {
assertNull(ctx.getHookContext().getException());
assertNotNull(ctx.getHookContext().getQueryInfo());
assertNotNull(ctx.getHookContext().getQueryInfo().getQueryDisplay());
+ assertQueryId(ctx.getQueryId());
}
}
+
+ /**
+ * Asserts that the specified query id exists and has the expected prefix and size.
+ *
+ * <p>
+ * A query id looks like below:
+ * <pre>
+ * username_20220330093338_dab90f30-5e79-463d-8359-0d2fff57effa
+ * </pre>
+ * and we can accurately predict how the prefix should look like. T
+ * </p>
+ *
+ * @param actualQueryId the query id to verify
+ */
+ private static void assertQueryId(String actualQueryId) {
+ assertNotNull(actualQueryId);
+ String expectedIdPrefix = makeQueryIdStablePrefix();
+ String actualIdPrefix = actualQueryId.substring(0, expectedIdPrefix.length());
+ assertEquals(expectedIdPrefix, actualIdPrefix);
+ assertEquals(expectedIdPrefix.length() + 41, actualQueryId.length());
+ }
+
+ /**
+ * Makes a query id prefix that is stable for an hour. The prefix changes every hour but this is enough to guarantee
Review comment:
@kasakrisz Thanks for pointing out the `hive.query.id` property, I wasn't aware of it. It seems that if we have this property then we can obtain the queryId in before/after hook methods by querying the configuration. Apparently we don't necessarily need the new API introduced in this PR (apart from convenience) so I am inclined to close this without merging. WDYT?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kasakrisz commented on a change in pull request #3156: HIVE-26095: Add queryid in QueryLifeTimeHookContext
Posted by GitBox <gi...@apache.org>.
kasakrisz commented on a change in pull request #3156:
URL: https://github.com/apache/hive/pull/3156#discussion_r840236630
##########
File path: service/src/test/org/apache/hive/service/cli/operation/TestQueryLifeTimeHooksWithSQLOperation.java
##########
@@ -110,6 +118,45 @@ public void afterExecution(QueryLifeTimeHookContext ctx, boolean hasError) {
assertNull(ctx.getHookContext().getException());
assertNotNull(ctx.getHookContext().getQueryInfo());
assertNotNull(ctx.getHookContext().getQueryInfo().getQueryDisplay());
+ assertQueryId(ctx.getQueryId());
}
}
+
+ /**
+ * Asserts that the specified query id exists and has the expected prefix and size.
+ *
+ * <p>
+ * A query id looks like below:
+ * <pre>
+ * username_20220330093338_dab90f30-5e79-463d-8359-0d2fff57effa
+ * </pre>
+ * and we can accurately predict how the prefix should look like. T
+ * </p>
+ *
+ * @param actualQueryId the query id to verify
+ */
+ private static void assertQueryId(String actualQueryId) {
+ assertNotNull(actualQueryId);
+ String expectedIdPrefix = makeQueryIdStablePrefix();
+ String actualIdPrefix = actualQueryId.substring(0, expectedIdPrefix.length());
+ assertEquals(expectedIdPrefix, actualIdPrefix);
+ assertEquals(expectedIdPrefix.length() + 41, actualQueryId.length());
+ }
+
+ /**
+ * Makes a query id prefix that is stable for an hour. The prefix changes every hour but this is enough to guarantee
Review comment:
IIUC `TestQueryLifeTimeHooksWithSQLOperation` tests whether data is passed to `QueryLifeTimeHookWithParseHooks`. I think the test is more isolated and focus only to the data broadcasting functionality if the data is generated by the test. It can be achieved by setting the queryId explicitly in the beginning of the test to some constant and check it in the assertion part the exact same constant. I believe it can be set by `hive.query.id` config setting.
Or you can also ask the actual queryId via the `hive.query.id` config setting somewhere in the beginning of the test and use that value in the assert part instead of generating a new one.
Or as a last option if none of above works a regex can be used to check the format.
If you also want to test the queryId format is correct an UT for `QueryPlan.makeQueryId` would do that but I think a regex is still needed for that or at least for checking the date time part.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] amansinha100 commented on a change in pull request #3156: HIVE-26095: Add queryid in QueryLifeTimeHookContext
Posted by GitBox <gi...@apache.org>.
amansinha100 commented on a change in pull request #3156:
URL: https://github.com/apache/hive/pull/3156#discussion_r840116428
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/hooks/QueryLifeTimeHookContextImpl.java
##########
@@ -78,11 +88,12 @@ public Builder withHookContext(HookContext hc) {
return this;
}
- public QueryLifeTimeHookContextImpl build() {
+ public QueryLifeTimeHookContextImpl build(String queryId) {
QueryLifeTimeHookContextImpl queryLifeTimeHookContext = new QueryLifeTimeHookContextImpl();
queryLifeTimeHookContext.setHiveConf(this.conf);
queryLifeTimeHookContext.setCommand(this.command);
queryLifeTimeHookContext.setHookContext(this.hc);
+ queryLifeTimeHookContext.queryId = Objects.requireNonNull(queryId);
Review comment:
Yes, sanity check was the only ask. Changes LGTM.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a change in pull request #3156: HIVE-26095: Add queryid in QueryLifeTimeHookContext
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #3156:
URL: https://github.com/apache/hive/pull/3156#discussion_r839977437
##########
File path: service/src/test/org/apache/hive/service/cli/operation/TestQueryLifeTimeHooksWithSQLOperation.java
##########
@@ -110,6 +118,45 @@ public void afterExecution(QueryLifeTimeHookContext ctx, boolean hasError) {
assertNull(ctx.getHookContext().getException());
assertNotNull(ctx.getHookContext().getQueryInfo());
assertNotNull(ctx.getHookContext().getQueryInfo().getQueryDisplay());
+ assertQueryId(ctx.getQueryId());
}
}
+
+ /**
+ * Asserts that the specified query id exists and has the expected prefix and size.
+ *
+ * <p>
+ * A query id looks like below:
+ * <pre>
+ * username_20220330093338_dab90f30-5e79-463d-8359-0d2fff57effa
+ * </pre>
+ * and we can accurately predict how the prefix should look like. T
+ * </p>
+ *
+ * @param actualQueryId the query id to verify
+ */
+ private static void assertQueryId(String actualQueryId) {
+ assertNotNull(actualQueryId);
+ String expectedIdPrefix = makeQueryIdStablePrefix();
+ String actualIdPrefix = actualQueryId.substring(0, expectedIdPrefix.length());
+ assertEquals(expectedIdPrefix, actualIdPrefix);
+ assertEquals(expectedIdPrefix.length() + 41, actualQueryId.length());
+ }
+
+ /**
+ * Makes a query id prefix that is stable for an hour. The prefix changes every hour but this is enough to guarantee
Review comment:
To avoid the risk we can remove the prefix check and keep only the length check. Alternatively, we can simply `assertNotNull` for the queryId and nothing more. What do you prefer?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a change in pull request #3156: HIVE-26095: Add queryid in QueryLifeTimeHookContext
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #3156:
URL: https://github.com/apache/hive/pull/3156#discussion_r839446442
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/hooks/QueryLifeTimeHookContextImpl.java
##########
@@ -78,11 +88,12 @@ public Builder withHookContext(HookContext hc) {
return this;
}
- public QueryLifeTimeHookContextImpl build() {
+ public QueryLifeTimeHookContextImpl build(String queryId) {
QueryLifeTimeHookContextImpl queryLifeTimeHookContext = new QueryLifeTimeHookContextImpl();
queryLifeTimeHookContext.setHiveConf(this.conf);
queryLifeTimeHookContext.setCommand(this.command);
queryLifeTimeHookContext.setHookContext(this.hc);
+ queryLifeTimeHookContext.queryId = Objects.requireNonNull(queryId);
Review comment:
We can enforce a min length check but its not easy to have a max since the query id includes the user name. The username max length is OS dependent.
I added a few more checks in https://github.com/apache/hive/pull/3156/commits/aae20a1fd3e9ae289daa3e6c95c20ae8a209211f. These are mostly sanity checks. I don't think we should put very involved checks here. Asserting things for the shape of the query id should be done via unit tests.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] amansinha100 commented on a change in pull request #3156: HIVE-26095: Add queryid in QueryLifeTimeHookContext
Posted by GitBox <gi...@apache.org>.
amansinha100 commented on a change in pull request #3156:
URL: https://github.com/apache/hive/pull/3156#discussion_r838910300
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/hooks/QueryLifeTimeHookContextImpl.java
##########
@@ -78,11 +88,12 @@ public Builder withHookContext(HookContext hc) {
return this;
}
- public QueryLifeTimeHookContextImpl build() {
+ public QueryLifeTimeHookContextImpl build(String queryId) {
QueryLifeTimeHookContextImpl queryLifeTimeHookContext = new QueryLifeTimeHookContextImpl();
queryLifeTimeHookContext.setHiveConf(this.conf);
queryLifeTimeHookContext.setCommand(this.command);
queryLifeTimeHookContext.setHookContext(this.hc);
+ queryLifeTimeHookContext.queryId = Objects.requireNonNull(queryId);
Review comment:
Should this also check this is not an empty string and is less than max length of the query id (is there an internal constant for 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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kasakrisz commented on a change in pull request #3156: HIVE-26095: Add queryid in QueryLifeTimeHookContext
Posted by GitBox <gi...@apache.org>.
kasakrisz commented on a change in pull request #3156:
URL: https://github.com/apache/hive/pull/3156#discussion_r839891772
##########
File path: service/src/test/org/apache/hive/service/cli/operation/TestQueryLifeTimeHooksWithSQLOperation.java
##########
@@ -110,6 +118,45 @@ public void afterExecution(QueryLifeTimeHookContext ctx, boolean hasError) {
assertNull(ctx.getHookContext().getException());
assertNotNull(ctx.getHookContext().getQueryInfo());
assertNotNull(ctx.getHookContext().getQueryInfo().getQueryDisplay());
+ assertQueryId(ctx.getQueryId());
}
}
+
+ /**
+ * Asserts that the specified query id exists and has the expected prefix and size.
+ *
+ * <p>
+ * A query id looks like below:
+ * <pre>
+ * username_20220330093338_dab90f30-5e79-463d-8359-0d2fff57effa
+ * </pre>
+ * and we can accurately predict how the prefix should look like. T
+ * </p>
+ *
+ * @param actualQueryId the query id to verify
+ */
+ private static void assertQueryId(String actualQueryId) {
+ assertNotNull(actualQueryId);
+ String expectedIdPrefix = makeQueryIdStablePrefix();
+ String actualIdPrefix = actualQueryId.substring(0, expectedIdPrefix.length());
+ assertEquals(expectedIdPrefix, actualIdPrefix);
+ assertEquals(expectedIdPrefix.length() + 41, actualQueryId.length());
+ }
+
+ /**
+ * Makes a query id prefix that is stable for an hour. The prefix changes every hour but this is enough to guarantee
Review comment:
What happens if the test starts at let's say 2022.03.31 01:59:59.999 and this method is called at 2022.03.31 02:00:00.000 ?
Does the query id format should be checked here? How about adding an UT for the query generator itself.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kasakrisz commented on a change in pull request #3156: HIVE-26095: Add queryid in QueryLifeTimeHookContext
Posted by GitBox <gi...@apache.org>.
kasakrisz commented on a change in pull request #3156:
URL: https://github.com/apache/hive/pull/3156#discussion_r840236630
##########
File path: service/src/test/org/apache/hive/service/cli/operation/TestQueryLifeTimeHooksWithSQLOperation.java
##########
@@ -110,6 +118,45 @@ public void afterExecution(QueryLifeTimeHookContext ctx, boolean hasError) {
assertNull(ctx.getHookContext().getException());
assertNotNull(ctx.getHookContext().getQueryInfo());
assertNotNull(ctx.getHookContext().getQueryInfo().getQueryDisplay());
+ assertQueryId(ctx.getQueryId());
}
}
+
+ /**
+ * Asserts that the specified query id exists and has the expected prefix and size.
+ *
+ * <p>
+ * A query id looks like below:
+ * <pre>
+ * username_20220330093338_dab90f30-5e79-463d-8359-0d2fff57effa
+ * </pre>
+ * and we can accurately predict how the prefix should look like. T
+ * </p>
+ *
+ * @param actualQueryId the query id to verify
+ */
+ private static void assertQueryId(String actualQueryId) {
+ assertNotNull(actualQueryId);
+ String expectedIdPrefix = makeQueryIdStablePrefix();
+ String actualIdPrefix = actualQueryId.substring(0, expectedIdPrefix.length());
+ assertEquals(expectedIdPrefix, actualIdPrefix);
+ assertEquals(expectedIdPrefix.length() + 41, actualQueryId.length());
+ }
+
+ /**
+ * Makes a query id prefix that is stable for an hour. The prefix changes every hour but this is enough to guarantee
Review comment:
IUUC `TestQueryLifeTimeHooksWithSQLOperation` tests whether data is passed to `QueryLifeTimeHookWithParseHooks`. I think the test is more isolated and focus only to the data broadcasting functionality if the data is generated by the test. It can be achieved by setting the queryId explicitly in the beginning of the test to some constant and check it in the assertion part the exact same constant. I believe it can be set by `hive.query.id` config setting.
Or you can also ask the actual queryId via the `hive.query.id` config setting somewhere in the beginning of the test and use that value in the assert part instead of generating a new one.
Or as a last option if none of above works a regex can be used to check the format.
If you also want to test the queryId format is correct an UT for `QueryPlan.makeQueryId` would do that but I think a regex is still needed for that or at least for checking the date time part.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org