Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-5296 : Ensure store file reader refcount is zero at end of relevant unit tests - #1096
Closed
virajjasani wants to merge 1 commit into
Closed
PHOENIX-5296 : Ensure store file reader refcount is zero at end of relevant unit tests#1096virajjasani wants to merge 1 commit into
virajjasani wants to merge 1 commit into
Conversation
stoty
commented
Jan 18, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
virajjasani
commented
Jan 18, 2021
ContributorAuthor
@stoty could you please take a look while we have 2nd QA results coming up? |
stoty
suggested changes
Jan 18, 2021
| thread.setDaemon(true); | ||
| thread.start(); | ||
| } | ||
Contributor
There was a problem hiding this comment.
Please move this to CompatUtil, the HbaseCompatCapabilities is for bool flags only.
stoty
suggested changes
Jan 18, 2021
| * | ||
| * @throws IOException if refCount is leaked | ||
| */ | ||
| protected synchronized static void confirmStoreRefCountLeak() |
Contributor
There was a problem hiding this comment.
I think that throwing an exception here may interfere with the cluster shutdown logic that this is called from.
Either catch and log it (and fail the test), or make sure that it doesn't preclude cluster shutdown at every invocation point.
ContributorAuthor
There was a problem hiding this comment.
I think better catch and log it and also throw AssertionError with explanation.
Contributor
There was a problem hiding this comment.
Throwing any exception would also stop the minicluster shutdown process, unless you catch it at the caller.
We should make sure that shutdownMiniCluster() will get called even if confirmStoreRefCountLeak() fails.
@AfterClass
public static void shutdown() {
confirmStoreRefCountLeak();
shutdownMiniCluster();
}
ContributorAuthor
There was a problem hiding this comment.
Updated PR should work. It does free up resources including shutting down mini or distributed cluster as per freeResources().
ContributorAuthor
There was a problem hiding this comment.
Let me update 4.x PR in the meanwhile.
stoty
commented
Jan 18, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
reviewed
Jan 18, 2021
stoty
left a comment
Contributor
There was a problem hiding this comment.
Maybe I'm being paranoiud, the given the amount of trouble miniCluster shutdown/startup has already caused for it, I'd like to make sure that we do not skip it anywhere.
(I didn't comment on each file, but we have quite a lot of them).
| CompatUtil.confirmStoreRefCountLeak(UTIL.getAdmin()); | ||
| } catch (IOException e) { | ||
| LOGGER.error("StoreFile refCount is leaked", e); | ||
| UTIL.shutdownMiniHBaseCluster(); |
Contributor
There was a problem hiding this comment.
Should just put these statements into a finally instead.
| hbaseTestUtil.getAdmin()); | ||
| } catch (IOException e) { | ||
| LOGGER.error("StoreFile refCount is leaked", e); | ||
| try { |
| tableName=null; | ||
| public void cleanUp() throws Exception { | ||
| confirmStoreRefCountLeak(); | ||
| tableName = null; |
| public void cleanUp() throws SQLException { | ||
| public void cleanUp() throws Exception { | ||
| confirmStoreRefCountLeak(); | ||
| deleteTenantData(descViewName); |
Contributor
There was a problem hiding this comment.
The should probably go into a finally as well.
ContributorAuthor
There was a problem hiding this comment.
This too should be taken care of by BaseTest#confirmStoreRefCountLeak() because it internally frees up resources and while doing so, drops all tables before shutting down cluster.
| @AfterClass | ||
| public static synchronized void teardown() { | ||
| public static synchronized void teardown() throws Exception { | ||
| confirmStoreRefCountLeak(); |
Contributor
There was a problem hiding this comment.
this should also be
try {
confirmStore...
} catch (Exception e) {
Assert.fail()}
} finally {
tearDownMiniCluster...
}
ContributorAuthor
There was a problem hiding this comment.
Oh btw we don't need to worry about putting anything in finally after calling BaseTest#confirmStoreRefCountLeak() because BaseTest is taking care of shutting down miniCluster:
protected synchronized static void confirmStoreRefCountLeak()
throws Exception {
if (getUtility() != null) {
try {
CompatUtil.confirmStoreRefCountLeak(
getUtility().getAdmin());
} catch (IOException e) {
LOGGER.error("StoreFile refCount is leaked", e);
freeResources(false);
fail("StoreFile refCount is leaked");
}
}
}
freeResources() is doing the job.
virajjasani
commented
Jan 18, 2021
ContributorAuthor
Definitely good point. Let me take a pass through all tests. |
stoty
reviewed
Jan 18, 2021
| CompatUtil.confirmStoreRefCountLeak( | ||
| getUtility().getAdmin()); | ||
| } catch (IOException e) { | ||
| LOGGER.error("StoreFile refCount is leaked", e); |
Contributor
There was a problem hiding this comment.
This is a rather unexpected side effect. I am not a fan of having the freeResources logic called from a different place in the normal and the error path.
Maybe call this method freeResourcesAndCheckRefCountLeak, and
call freeResources unconditionally from from finally here ?
ContributorAuthor
There was a problem hiding this comment.
Okk I think we better let cluster shutdown happen the way it does today. We better use finally with catching storeFile refCount leak assertion catch so that not only we fail test but also let resources be freed up in individual test's finally block. I was thinking of taking care of cluster shutdown in BaseTest only as generic check, but now it seems better let individual test decide what they want to free up - cluster or just drop table or anything as per it's current behaviour.
The only problem with current approach (generic freeup resource in BaseTest) is that if refCount leakage is encountered in one test and if we shutdown miniCluster in @After, other tests in that class will not run anyways.
@stoty you are also fine with having finally in each test right?
stoty
commented
Jan 18, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 18, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 18, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 18, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 19, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
approved these changes
Jan 19, 2021
stoty
left a comment
Contributor
There was a problem hiding this comment.
LGTM.
This is certain not to disrupt the control flow.
virajjasani
commented
Jan 19, 2021
ContributorAuthor
Let me resolve the conflicts |
virajjasani
commented
Jan 20, 2021
ContributorAuthor
Build is timing out, not sure if relevant. |
stoty
commented
Jan 20, 2021
Contributor
Yout may want to rebase these on HEAD, in case the Xmx setting is the culprit. |
virajjasani
commented
Jan 20, 2021
ContributorAuthor
Let me also upload patch over Jira in the meanwhile. |
stoty
commented
Jan 20, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 20, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 20, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 22, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 22, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 22, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 22, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 22, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 24, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
virajjasani
commented
Jan 24, 2021
ContributorAuthor
@stoty last 2 builds have |
virajjasani
commented
Jan 25, 2021
ContributorAuthor
@stoty i have done small change in |
stoty
reviewed
Jan 25, 2021
| <version>${slf4j.version}</version> | ||
| <scope>provided</scope> | ||
| </dependency> | ||
| <dependency> |
Contributor
There was a problem hiding this comment.
Could you solve this without Guava ?
I know that it doesn't really matter, but it pains me to see anther Guava dependency added.
ContributorAuthor
There was a problem hiding this comment.
For 4.x Uninterruptibles is in beta version based on guava version that we use so i used this: https://github.com/apache/phoenix/pull/1097/files#diff-d12329d4796498b9880a54bb4b6d681f7ee42b924536bf5247791f372329cd9fR61
However, i feel using Uninterruptibles.sleepUninterruptibly more comfortably as we don't have to catch InterruptedException and it is not interruptible. But i agree with your point on not introducing more dependency so let me replicate same logic as 4.x.
stoty
commented
Jan 25, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
virajjasani
commented
Jan 25, 2021
ContributorAuthor
@stoty Thanks for the suggestion, i have updated PR for |
stoty
commented
Jan 26, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 26, 2021
Contributor
One more nit: Please update the commit message to match the JIRA description. |
stoty
commented
Jan 26, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 27, 2021
Contributor
Committed with update commit message. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.