Backport HBASE-23553 to branch-2.1 #1230
Conversation
HBASE-23553 Snapshot referenced data files are deleted in some case
This comment has been minimized.
This comment has been minimized.
|
Are you sure the test still has coverage for the issue in HBASE-23553? If I apply the changes locally the test passes with and without the relevant changes to |
| Path rootDir = UTIL.getHBaseCluster().getMaster().getMasterFileSystem().getRootDir(); | ||
| long timeout = 20000; // 20s | ||
| try (Admin admin = UTIL.getAdmin()) { | ||
| List<String> serverList = admin.getRegionServers().stream().map(sn -> sn.getServerName()) |
| return false; | ||
| } | ||
| }); | ||
| // run catalog janitor to clean and wait for parent regions are archived |
There was a problem hiding this comment.
Nit: IIRC, we call the input to a merge the "daughters" of a merge which come together to make one "parent". It's like split (e.g one parent, two daughters), but in reverse.
| } | ||
| }); | ||
| // set file modify time and then run cleaner | ||
| long time = System.currentTimeMillis() - TimeToLiveHFileCleaner.DEFAULT_TTL * 1000; |
There was a problem hiding this comment.
Can we pull the actual TTL out of the configuration instead of relying on this default ttl variable?
| } catch (Exception e) { | ||
| LOG.error("scan snapshot error", e); | ||
| Assert.fail("Should not throw FileNotFoundException"); | ||
| Assert.assertTrue(e.getCause() != null); |
| } | ||
| } catch (Exception e) { | ||
| LOG.error("scan snapshot error", e); | ||
| Assert.fail("Should not throw FileNotFoundException"); |
There was a problem hiding this comment.
Don't you really want to make sure just the TableSnapshotScanner doesnt' throw a FileNotFoundException? Like, maybe lift this catch up into that try block specifically?
|
note for committers if I'm not the one who pushes this, the following commit message should be used: Karthik I used the email address associated with your github account above. let us know if you'd prefer a different one. |
|
Sorry @joshelser. Its my bad! I simply back-ported theses changes, just only excluding compactionSwitch in the code. I should have done review before committing. Pls review the latest commit. ||Are you sure the test still has coverage for the issue in HBASE-23553? Usually, we don't need compactionSwitch in this test case, as we know it never triggers compaction since it only loads less data with no manual compaction and only an individual test. |
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestTableSnapshotScanner.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
If there's test coverage then how do I run a unit test to get a failure prior to the fix? On the original backport I applied just the test changes and then ran those tests. They passed. What am I missing? |
There was a problem hiding this comment.
I grabbed things as of 7055189 and now I get a failure of the new test without the fix as expected.
I'll go ahead and merge. Thanks @karthikhw and thanks @joshelser for the additional review.
Backport to branch-2.1 via HBASE-23915 by Karthik P. Differs from original by skipping test-only need for the "turn compaction on/off" feature. Closes #1230 Co-authored-by: Karthik Palanisamy <kpalanisamy@hortonworks.com> Signed-off-by: Josh Elser <elserj@apache.org> Signed-off-by: Sean Busbey <busbey@apache.org>
HBASE-23553 Snapshot referenced data files are deleted in some case