kabhishek4 commented on code in PR #5605:
URL: https://github.com/apache/hbase/pull/5605#discussion_r1553937551
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java:
##########
@@ -336,6 +337,62 @@ 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 {
+ // Configure custom delay
+ PrefetchExecutorNotifier prefetchExecutorNotifier = new
PrefetchExecutorNotifier(conf);
+ conf.setInt(PREFETCH_DELAY, 25000);
+ prefetchExecutorNotifier.onConfigurationChange(conf);
+
+ HFileContext context = new
HFileContextBuilder().withCompression(Compression.Algorithm.GZ)
+ .withBlockSize(DATA_BLOCK_SIZE).build();
+ Path storeFile = writeStoreFile("TestPrefetchWithDelay", context);
+
+ HFile.Reader reader = HFile.createReader(fs, storeFile, cacheConf, true,
conf);
+ long startTime = System.currentTimeMillis();
+
+ // Wait for 20 seconds, no thread should start prefetch
+ Thread.sleep(20000);
+ assertFalse("Prefetch threads should not be running at this point",
reader.prefetchStarted());
+ while (!reader.prefetchStarted()) {
+ assertTrue("Prefetch delay has not been expired yet",
+ getElapsedTime(startTime) < PrefetchExecutor.getPrefetchDelay());
+ }
+
+ // Prefech threads started working but not completed yet
+ assertFalse(reader.prefetchComplete());
+
+ // In prefetch executor, we further compute passed in delay using
variation and a random
+ // multiplier to get 'effective delay'. Hence, in the test, for delay of
25000 milli-secs
+ // check that prefetch is started after 20000 milli-sec and prefetch
started after that.
+ // However, prefetch should not start after configured delay.
+ if (reader.prefetchStarted()) {
+ LOG.info("elapsed time {}, Delay {}", getElapsedTime(startTime),
+ PrefetchExecutor.getPrefetchDelay());
+ assertTrue("Prefetch should start post configured delay",
+ getElapsedTime(startTime) <= PrefetchExecutor.getPrefetchDelay());
+ }
Review Comment:
I agree that setting the variation to 0 would remove the random seed and
delay would be deterministic. However, the cause for value of variation is not
getting set is seems to be different instances of conf in the test and in the
static initializer block of prefetch executor.
static {
// Consider doing this on demand with a configuration passed in rather
// than in a static initializer.
Configuration conf = HBaseConfiguration.create();
With new changes if the prefetch delay is set, it's value get reflected
becuase conf is getting reloaded which is not the case with the variation.
I am contemplating to code below to address this. Please let me know if you
agree with the approach.
1. Add new test only method to set the prefetchDelayVariation in the
PrefetchExecutor class
2. Call it from the test to set to 0 and to unset
3. With this prefetchDelayVariation cannot be declared as final in the
prefetch executor class
4. Also, I am starting the timer abit late i.e.
HFile.Reader reader = HFile.createReader(fs, storeFile, cacheConf, true,
conf);
long startTime = System.currentTimeMillis();
hence wait for small amount of time, say 500 millisecs before testing
the assertions.
--
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]
