wchevreuil commented on code in PR #5605:
URL: https://github.com/apache/hbase/pull/5605#discussion_r1537538033
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java:
##########
@@ -295,10 +302,23 @@ private void readStoreFile(Path storeFilePath,
throws Exception {
// Open the file
HFile.Reader reader = HFile.createReader(fs, storeFilePath, cacheConfig,
true, conf);
+ startTimer();
while (!reader.prefetchComplete()) {
// Sleep for a bit
Thread.sleep(1000);
+ if (getComputeTiming()) {
+ // After task is scheduled and before the delay expires, prefetch
should not start
+ // if prefetchFutures contains entry (which means it's not cancelled
or completed)
+ // and wait time remaining is below delay expiry watermark, it can be
deduced that
+ // the prefetch is not started yet.
+ if (getElapsedTime() >= (conf.getLong(PREFETCH_DELAY, 1000))) {
+ assertTrue("Prefetch should be started at this point",
reader.prefetchStarted());
+ setComputeTiming(false);
+ } else {
+ assertFalse("Prefetch Should not start at this point",
reader.prefetchStarted());
+ }
+ }
Review Comment:
This version of readStoreFile enforces the prefetch execution and
completion. This is relevant to some of the tests in this class, but definitely
not for yours.
You should rather just move the reader creation line to your test method,
then put your validation logic there. That would yield the following benefits:
1) Avoid the need for the additional global variables that are only of
concern of a single test method (anti pattern);
2) Make your test easier to be understood by other readers;
3) Make your test more cohesive, and quicker, as it doesn't need to wait for
the whole prefetch completion.
4) Keep existing version of readStoreFile more cohesive, as it doesn't
implement additional variations of behaviour.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java:
##########
@@ -96,6 +99,10 @@ public class TestPrefetch {
private static final int DATA_BLOCK_SIZE = 2048;
private static final int NUM_KV = 1000;
+ private long startTime;
+ private boolean measureTiming;
+
Review Comment:
Please don't add global variables that are only used by one specific test
logic.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java:
##########
@@ -336,6 +356,42 @@ public void testPrefetchDoesntSkipRefs() throws Exception {
});
}
+ @Test
+ public void testOnConfigurationChange() {
+ PrefetchExecutorNotifier prefetchExecutorNotifier = new
PrefetchExecutorNotifier(conf);
+ conf.setInt(PREFETCH_DELAY, 40000);
+ prefetchExecutorNotifier.onConfigurationChange(conf);
+ assertEquals(prefetchExecutorNotifier.getPrefetchDelay(), 40000);
+
+ // restore
+ conf.setInt(PREFETCH_DELAY, 30000);
+ prefetchExecutorNotifier.onConfigurationChange(conf);
+ assertEquals(prefetchExecutorNotifier.getPrefetchDelay(), 30000);
+
+ conf.setInt(PREFETCH_DELAY, 1000);
+ prefetchExecutorNotifier.onConfigurationChange(conf);
+ }
+
+ @Test
+ public void testPrefetchWithDelay() throws Exception {
+ PrefetchExecutorNotifier prefetchExecutorNotifier = new
PrefetchExecutorNotifier(conf);
+ conf.setInt(PREFETCH_DELAY, 35000);
+ prefetchExecutorNotifier.onConfigurationChange(conf);
+
+ HFileContext context = new
HFileContextBuilder().withCompression(Compression.Algorithm.GZ)
+ .withBlockSize(DATA_BLOCK_SIZE).build();
+ Path storeFile = writeStoreFile("TestPrefetchWithDelay", context);
+
+ resetTiming();
+ setComputeTiming(true);
+
+ readStoreFile(storeFile);
Review Comment:
We don't need the whole logic of readStoreFile here. Please see my comment
above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
