Skip to content

Conversation

@veghlaci05
Copy link
Contributor

What changes were proposed in this pull request?

Fix the flaky test:
TestHiveMetaStoreTimeout#testResetTimeout

Why are the changes needed?

This test was sensitive to network/DB speed, and slow CI builds were resulted in unexpected timeouts.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By running TestHiveMetaStoreTimeout#testResetTimeout manually


// no timeout before reset
client.dropDatabase(dbName, true, true);
HMSHandler.testTimeoutValue = 250;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get it how this config will resolve the issue. Could you please elaborate it?

Copy link
Contributor Author

@veghlaci05 veghlaci05 May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally the low timeout value was set at the beginning of the test. Normally it is high enough that client.dropDatabase(dbName, true, true); won't timeout, but client.createDatabase(db); will do. However, if the network/DB is very slow, even client.dropDatabase(dbName, true, true); can timeout causing the whole test to fail. Since the default value of the timeout is -1 (no timout) I moved the lowering the value after the dropdatabase call, and restored -1 after createDatabase fail, so now the test is no longer sensitive to the environment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setup method of the test enables timeout. What about playing with the timeout values, just disabling timeout at the beginning of the test case and re-enable it after the drop database command?

And also, I wonder why the test testNoTimeout is not flaky. It works with the same timeout value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I'll rewrite the test methods that way

@InvisibleProgrammer
Copy link
Contributor

LGTM. Init timeout failed at precommits so please restart the tests. Anyway, the test cases were all green so it seem we are good.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@veghlaci05 veghlaci05 merged commit 3fe7234 into apache:master May 19, 2023
@veghlaci05 veghlaci05 deleted the HIVE-27343 branch June 12, 2023 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants