-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modify TestDirectoryListCacheInvalidation to fix flakiness #22455
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ensured that the cluster under test has at most one test running against it?
long hitCount1 = (long) result.row(0).get(0); | ||
long missCount1 = (long) result.row(0).get(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
long hitCount1 = (long) result.row(0).get(0); | |
long missCount1 = (long) result.row(0).get(1); | |
long hitCount = (long) result.row(0).get(0); | |
long missCount = (long) result.row(0).get(1); |
77fb90e
to
f1b6cfb
Compare
You mean tests where |
Yes--can the query count metrics be influenced by any concurrent running queries? |
I think it could. Since here hitcount, missCount variable may not hold the value incremented by 1 which was captured here in https://github.com/prestodb/presto/pull/22455/files#diff-20e1ed420a5fd877893fcaacc97f3ad50d36e5ee71771a0d828a12c29dc19525R44. Since these metrics are for overall connector. |
@agrawalreetika does it make sense then, since these metrics may be influenced by other tests, to modify the assertions to check that the counts have updated by at least 1, instead of asserting hard equality on 1? |
@tdcmeehan That would help us in such cases for the test not to fail but it could avoid the test on the actual tests since we wouldn't know if metrics are incremented with respective tests. Else, we can update assertions to check that the counts have updated by at least 1 as you suggested earlier. LMK |
Having a dedicated catalog for this test actually makes a lot of sense to me. |
f1b6cfb
to
a90fb28
Compare
Thanks @agrawalreetika! |
Description
Modify TestDirectoryListCacheInvalidation to fix flakiness
Motivation and Context
TestDirectoryListCacheInvalidation test was failing because caching catalog
hivecached
is shared with https://github.com/prestodb/presto/blob/master/presto-product-tests/src/main/java/com/facebook/presto/tests/hive/TestSymlinkTableListCaching.java So the initial existing assert https://github.com/prestodb/presto/blob/master/presto-product-tests/src/main/java/com/facebook/presto/tests/hive/TestDirectoryListCacheInvalidation.java#L45 may not be true if TestSymlinkTableListCaching is running first. Since the JMX forhitcount
andmisscount
may already be populated as part of TestSymlinkTableListCachingImpact
Fixes #22425
Test Plan
Contributor checklist
Release Notes