HIVE-29274: Flaky TestMetastoreLeaseLeader#testHouseKeepingThreads #6142
Conversation
...unit/src/test/java/org/apache/hadoop/hive/metastore/MetastoreHousekeepingLeaderTestBase.java
Outdated
Show resolved
Hide resolved
|
|
||
| warehouse = new Warehouse(conf); | ||
|
|
||
| conf.set("metastore.leader.test.listener", TestLeaderNotification.class.getName()); |
There was a problem hiding this comment.
could we move this to constants?
There was a problem hiding this comment.
This property is for test only, it's shouldn't be exposed to user or else where in the unit test
There was a problem hiding this comment.
how is that different from LEADER_IN_TEST?: https://github.com/apache/hive/pull/6142/files#diff-d697cd0d86def896a111982bc9a1c1d4cdd1175fcc3a21599ce3f0e228613895R80
| threadClasses.forEach((thread, status) -> threadClasses.put(thread, false)); | ||
| } | ||
|
|
||
| public static class TestLeaderNotification implements LeaderElection.LeadershipStateListener { |
There was a problem hiding this comment.
for creating the instance from reflection, https://github.com/apache/hive/pull/6142/files#diff-d697cd0d86def896a111982bc9a1c1d4cdd1175fcc3a21599ce3f0e228613895R109, a public access of the class is needed
There was a problem hiding this comment.
using reflection in tests is typically not the best approach, as it often indicates underlying design issues
...ive-unit/src/test/java/org/apache/hadoop/hive/metastore/TestMetastoreHousekeepingLeader.java
Outdated
Show resolved
Hide resolved
| listenerMap.forEach((k, v) -> newListeners.get(TTYPE.HOUSEKEEPING).addAll(v)); | ||
| List<LeadershipStateListener> listeners = new ArrayList<>(); | ||
| listenerMap.forEach((k, v) -> listeners.addAll(v)); | ||
| String testListenerCls = conf.get("metastore.leader.test.listener"); |
There was a problem hiding this comment.
That is a very bad practice to bring test related things into production code! Try mocking/stubbing, etc..
There was a problem hiding this comment.
This is for the integration test, the mocking/stubbing I think couldn't imitate a HMS similar as the one deployed in production.
There was a problem hiding this comment.
Including test-specific workarounds in production code is generally discouraged, as it’s considered a code smell and not a best practice.
same applies conf.getBoolean(LEADER_IN_TEST, false)
There was a problem hiding this comment.
Agreed, IMO we need to trade off between the test coverage and the maintenance(code smell), and reach a balance. We also have some properties such as hive.in*.test for testing purpose.
There was a problem hiding this comment.
We also have some properties such as hive.in*.test for testing purpose
True — those are legacy and should be removed once all tests are refactored (I’ve already done this in several cases).
However, that’s not a reason to introduce more such configs. The code should be structured in a way that makes it easy to test using the existing test framework tools.
| notifyListener(); | ||
| } | ||
| if (!conf.getBoolean(METASTORE_RENEW_LEASE, true)) { | ||
| // For test only. |
...astore-server/src/main/java/org/apache/hadoop/hive/metastore/leader/LeaseLeaderElection.java
Outdated
Show resolved
Hide resolved
...astore-server/src/main/java/org/apache/hadoop/hive/metastore/leader/LeaseLeaderElection.java
Outdated
Show resolved
Hide resolved
...astore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java
Outdated
Show resolved
Hide resolved
...astore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java
Outdated
Show resolved
Hide resolved
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
asf-ci-hive
added
tests pending
tests failed
and removed
tests passed
tests pending
labels
|
3 participants
What changes were proposed in this pull request?
Why are the changes needed?
Does this PR introduce any user-facing change?
No
How was this patch tested?
https://ci.hive.apache.org/job/hive-flaky-check/915