thomasrebele commented on code in PR #6766:
URL: https://github.com/apache/hive/pull/6766#discussion_r3968818176
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/client/TestMetastoreScheduledQueries.java:
##########
@@ -395,6 +396,98 @@ public void testSkip2() throws Exception {
}
}
+
+ /**
+ * Simulates the HA scenario where MScheduledExecution#scheduledExecutionId
values (assigned by
+ * DataNucleus's per-instance pre-allocated "native" id blocks) do not
correlate with true
+ * completion order (tracked by endTime).
+ *
+ * Three executions are created for a single scheduled query with ids
assigned in natural,
+ * increasing creation order (exec1 < exec2 < exec3). exec1's and
exec2's endTime are then
+ * rewritten directly (through the same PersistenceManager machinery
ObjectStore itself uses) so
+ * their completion order is the reverse of their id order -- exactly the
symptom of the HA id
+ * pre-allocation bug -- without needing an actual multi-instance cluster.
Review Comment:
I think this can be reworded to make it a bit clearer and shorter. How about
sth like
> Three executions are created for a single scheduled query
> with ids in increasing creation order (exec1 < exec2 < exec3),
> but with exec2 before exec1 in the order of the endTime (exec2 < exec1
< exec3).
The "through the same PersistenceManager machinery ObjectStore itself uses"
is already mentioned in a comment of the method, so I would drop it here. Not
sure whether "without needing an actual multi-instance cluster" adds value
here, it sounds a bit like marketing to me.
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/client/TestMetastoreScheduledQueries.java:
##########
@@ -395,6 +396,98 @@ public void testSkip2() throws Exception {
}
}
+
+ /**
+ * Simulates the HA scenario where MScheduledExecution#scheduledExecutionId
values (assigned by
+ * DataNucleus's per-instance pre-allocated "native" id blocks) do not
correlate with true
+ * completion order (tracked by endTime).
+ *
+ * Three executions are created for a single scheduled query with ids
assigned in natural,
+ * increasing creation order (exec1 < exec2 < exec3). exec1's and
exec2's endTime are then
+ * rewritten directly (through the same PersistenceManager machinery
ObjectStore itself uses) so
+ * their completion order is the reverse of their id order -- exactly the
symptom of the HA id
+ * pre-allocation bug -- without needing an actual multi-instance cluster.
+ *
+ * With autoDisableCount=2, skipCount=0 (lastN=2):
+ * - Ordering by id descending (the old, buggy behavior) picks {exec3
FAILED, exec2 FAILED} as
+ * the "last 2 executions" -> 2 consecutive failures -> incorrectly
disabled.
+ * - Ordering by endTime (the fix) picks {exec1 FINISHED, exec3 FAILED} as
the "last 2
+ * executions" -> the FINISHED row breaks the failure streak ->
correctly NOT disabled.
+ *
+ * This test asserts the correct outcome, so it fails against the unfixed
+ * ObjectStore#processScheduledQueryPolicies and passes once its ordering
uses endTime instead
+ * of scheduledExecutionId.
Review Comment:
I would not describe the behavior of the previous code, but rather what the
expected outcome is. And what's `lastN` referring to? How about something along
the lines of:
> With autoDisableCount=2, skipCount=0, the ObjectStore algorithm looks at
the consecutive sequences of queries [exec2, exec1] and [exec1, exec3], and as
none of the sequences consists of only failures, the autodisable count does not
apply.
I'm not sure whether SCHEDULED_QUERIES_AUTODISABLE_COUNT works that way, but
that's how I interpret its documentation in MetastoreConf.java.
Also not sure whether that needs to be part of the test method's javadoc, or
whether that should be a comment of the assertion.
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/client/TestMetastoreScheduledQueries.java:
##########
@@ -395,6 +396,98 @@ public void testSkip2() throws Exception {
}
}
+
+ /**
+ * Simulates the HA scenario where MScheduledExecution#scheduledExecutionId
values (assigned by
+ * DataNucleus's per-instance pre-allocated "native" id blocks) do not
correlate with true
+ * completion order (tracked by endTime).
+ *
+ * Three executions are created for a single scheduled query with ids
assigned in natural,
+ * increasing creation order (exec1 < exec2 < exec3). exec1's and
exec2's endTime are then
+ * rewritten directly (through the same PersistenceManager machinery
ObjectStore itself uses) so
+ * their completion order is the reverse of their id order -- exactly the
symptom of the HA id
+ * pre-allocation bug -- without needing an actual multi-instance cluster.
+ *
+ * With autoDisableCount=2, skipCount=0 (lastN=2):
+ * - Ordering by id descending (the old, buggy behavior) picks {exec3
FAILED, exec2 FAILED} as
+ * the "last 2 executions" -> 2 consecutive failures -> incorrectly
disabled.
+ * - Ordering by endTime (the fix) picks {exec1 FINISHED, exec3 FAILED} as
the "last 2
+ * executions" -> the FINISHED row breaks the failure streak ->
correctly NOT disabled.
+ *
+ * This test asserts the correct outcome, so it fails against the unfixed
+ * ObjectStore#processScheduledQueryPolicies and passes once its ordering
uses endTime instead
+ * of scheduledExecutionId.
+ */
+ @Test
+ public void testDisablePolicyUsesEndTimeNotExecutionIdForOrdering() throws
Exception {
+ String testNamespace = "haorderskew";
+
metaStore.getConf().set(MetastoreConf.ConfVars.SCHEDULED_QUERIES_AUTODISABLE_COUNT.getVarname(),
"2");
+
metaStore.getConf().set(MetastoreConf.ConfVars.SCHEDULED_QUERIES_SKIP_OPPORTUNITIES_AFTER_FAILURES.getVarname(),
+ "0");
+ client.close();
+ client = metaStore.getClient();
+
+ ScheduledQueryKey schqKey = new ScheduledQueryKey("q1", testNamespace);
+ createEverySecondSchq(schqKey);
+
+ // exec1: finishes successfully; naturally gets the smallest id of the
three.
+ long exec1Id = pollAndGetExecutionId(testNamespace);
+ client.scheduledQueryProgress(
+ new ScheduledQueryProgressInfo(exec1Id, QueryState.FINISHED,
"executor-query-id"));
+
+ // exec2: fails; naturally gets a bigger id than exec1.
+ long exec2Id = pollAndGetExecutionId(testNamespace);
+ ScheduledQueryProgressInfo exec2Info =
+ new ScheduledQueryProgressInfo(exec2Id, QueryState.FAILED,
"executor-query-id");
+ exec2Info.setErrorMessage("some issue happened");
+ client.scheduledQueryProgress(exec2Info);
+
+ // Simulate HA id/endTime skew: rewrite endTime directly (through the same
JDO machinery
+ // ObjectStore itself uses) so exec1 (lowest id) looks like it completed
most recently, and
+ // exec2 (higher id) looks like it completed long ago.
+ int skewedRecentEndTime = getEpochSeconds();
+ try (PersistenceManager pm =
PersistenceManagerProvider.getPersistenceManager()) {
+ Transaction tx = pm.currentTransaction();
+ tx.begin();
+ MScheduledExecution exec1 = pm.getObjectById(MScheduledExecution.class,
exec1Id);
+ exec1.setEndTime(skewedRecentEndTime);
+ MScheduledExecution exec2 = pm.getObjectById(MScheduledExecution.class,
exec2Id);
+ exec2.setEndTime(1);
+ tx.commit();
+ }
+
+ // exec3: fails; naturally gets the biggest id. Its real endTime (captured
inside
+ // scheduledQueryProgress at report time, below) is >=
skewedRecentEndTime, since real epoch
+ // time only moves forward from the point it was captured above.
+ long exec3Id = pollAndGetExecutionId(testNamespace);
+ ScheduledQueryProgressInfo exec3Info =
+ new ScheduledQueryProgressInfo(exec3Id, QueryState.FAILED,
"executor-query-id");
+ exec3Info.setErrorMessage("some issue happened");
+ client.scheduledQueryProgress(exec3Info);
+
+ // Correct (endTime-ordered) last-2-executions are {exec1 FINISHED, exec3
FAILED}: only 1
+ // consecutive failure -> must NOT be disabled. (Buggy id-ordering would
instead see
+ // {exec3 FAILED, exec2 FAILED}: 2 consecutive failures -> incorrectly
disabled.)
+ ScheduledQuery schq = client.getScheduledQuery(schqKey);
+ assertTrue("Scheduled query must remain enabled: true completion order has
only 1 "
+ + "consecutive failure, despite exec2 (an older failure by real
completion time) "
+ + "having a numerically higher id than exec1", schq.isEnabled());
+ }
+
+ private long pollAndGetExecutionId(String testNamespace) throws Exception {
+ ScheduledQueryPollRequest request = new
ScheduledQueryPollRequest(testNamespace);
+ ScheduledQueryPollResponse pollResult = null;
+ for (int i = 0; i < 30; i++) {
+ pollResult = client.scheduledQueryPoll(request);
+ if (pollResult.isSetQuery()) {
+ break;
+ }
+ Thread.sleep(100);
+ }
+ assertTrue("expected a scheduled query execution to become available",
pollResult.isSetQuery());
Review Comment:
I wonder if this can introduce some flaky test issues. The other tests wait
for 1s, while here we wait 30*100ms=3s, so I guess it should be fine.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
