HIVE-30024: Fix scheduled queries for HA configuration - #6766
HIVE-30024: Fix scheduled queries for HA configuration#6766InvisibleProgrammer wants to merge 2 commits into
Conversation
thomasrebele
left a comment
thomasrebele
left a comment
There was a problem hiding this comment.
Thanks for the fix! The logic of the fix and test looks appropriate. The comments have lots of redundancies and sound a bit AI generated. If that's the case, please include "Generated-by:" or "Co-authored-by:" per the ASF Generative Tooling Guidance. I would suggest condensing the comments to the essential information.
| * 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. |
There was a problem hiding this 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.
| * 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. |
There was a problem hiding this 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.
There was a problem hiding this comment.
Agreed with @thomasrebele here, we should keep the javadoc precisely explaining what the test is about and preferably avoid explaining the buggy behaviour being fixed.
There was a problem hiding this comment.
Removed the extra comments to match the common commenting pattern, like at testSkip2.
There was a problem hiding this comment.
Also refactored the test itself.
| } | ||
| Thread.sleep(100); | ||
| } | ||
| assertTrue("expected a scheduled query execution to become available", pollResult.isSetQuery()); |
There was a problem hiding this 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.
There was a problem hiding this comment.
I used to ask for Awaitility for this kind of polling in tests, I would encourage you to use it, it's already in hive's pom
There was a problem hiding this comment.
@abstractdog , interesting suggestion. I'm not familiar with this library. Could you please explain the benefits of using it?
Thx.
There was a problem hiding this comment.
in my opinion:
- in a large codebase, you include it once, then reuse it instead of implementing the polling every time (with different quality)
- certain static code checks will nitpick all the time when it finds
Thread.sleep, some of them are relevant, some of them are not: with Awatility you push this noise/responsibility away from your code :)
I accidentally found it, and then I realized it's already in iceberg and hive
There was a problem hiding this comment.
Rewrote to use the suggested library.
d7a29eb to
8ec3983
Compare
asf-ci-hive
added
tests unstable
tests pending
and removed
tests pending
tests unstable
labels
|
5 participants
Fixes a scheduled query issue related to HA configurations. Details in the ticket.
What changes were proposed in this pull request?
We can use endTime instead of scheduledExecutionId to determine the order of the executions.
Why are the changes needed?
Because of the pre-allocation feature of DataNucleus
Does this PR introduce any user-facing change?
No
How was this patch tested?
Added a new unit test (TestMetastoreScheduledQueries#testDisablePolicyUsesEndTimeNotExecutionIdForOrdering) that simulates the behavior.