petersomogyi commented on code in PR #5605:
URL: https://github.com/apache/hbase/pull/5605#discussion_r1444797348
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java:
##########
@@ -20,6 +20,7 @@
import static
org.apache.hadoop.hbase.client.trace.hamcrest.SpanDataMatchers.hasName;
import static
org.apache.hadoop.hbase.client.trace.hamcrest.SpanDataMatchers.hasParentSpanId;
import static
org.apache.hadoop.hbase.io.hfile.CacheConfig.CACHE_DATA_BLOCKS_COMPRESSED_KEY;
+import static org.apache.hadoop.hbase.io.hfile.PrefetchExecutor.*;
Review Comment:
Don't use wildcard imports.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java:
##########
@@ -298,6 +299,44 @@ public void testPrefetchDoesntSkipRefs() throws Exception {
});
}
+ @Test
+ public void testOnConfigurationChange() {
+ // change PREFETCH_DELAY_ENABLE_KEY from false to true
+ conf.setInt(PREFETCH_DELAY, 40000);
+ PrefetchExecutor.loadConfiguration(conf);
+ assertTrue(getPrefetchDelay() == 40000);
Review Comment:
Use `assertEquals` instead for equation checks.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java:
##########
@@ -298,6 +299,44 @@ public void testPrefetchDoesntSkipRefs() throws Exception {
});
}
+ @Test
+ public void testOnConfigurationChange() {
+ // change PREFETCH_DELAY_ENABLE_KEY from false to true
+ conf.setInt(PREFETCH_DELAY, 40000);
+ PrefetchExecutor.loadConfiguration(conf);
+ assertTrue(getPrefetchDelay() == 40000);
+
+ // restore
+ conf.setInt(PREFETCH_DELAY, 30000);
+ PrefetchExecutor.loadConfiguration(conf);
+ assertTrue(getPrefetchDelay() == 30000);
+
+ }
+
+ @Test
+ public void testPrefetchWithDelay() throws Exception {
+ conf.setInt(PREFETCH_DELAY, 40000);
+ PrefetchExecutor.loadConfiguration(conf);
+
+ HFileContext context = new
HFileContextBuilder().withCompression(Compression.Algorithm.GZ)
+ .withBlockSize(DATA_BLOCK_SIZE).build();
+ Path storeFile = writeStoreFile("TestPrefetchWithDelay", context);
+ readStoreFile(storeFile);
+ assertTrue(getPrefetchDelay() == 40000);
+ }
+
+ @Test
+ public void testPrefetchWithDefaultDelay() throws Exception {
+ conf.setInt(PREFETCH_DELAY, 1000);
Review Comment:
Can you unset the property completely and test what value is picked up? This
way the test is identical with `testPrefetchWithDelay`.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/PrefetchExecutor.java:
##########
@@ -134,4 +139,25 @@ private PrefetchExecutor() {
static ScheduledExecutorService getExecutorPool() {
return prefetchExecutorPool;
}
+
+ public static int getPrefetchDelay() {
+ return prefetchDelayMillis;
+ }
+
+ @Override
+ public void onConfigurationChange(Configuration conf) {
+ PrefetchExecutor.loadConfiguration(conf);
+ }
+
+ @Override
+ public void registerChildren(ConfigurationManager manager) {
+ }
+
+ @Override
+ public void deregisterChildren(ConfigurationManager manager) {
+ }
+
+ public static void loadConfiguration(Configuration conf) {
+ prefetchDelayMillis = conf.getInt(PREFETCH_DELAY, 1000);
Review Comment:
Is this enough?
What happens if the PrefetchExecutor is started with a large value (e.g. 1
hour) and the operator updates the delay to 5 seconds? Wouldn't it wait for 1
hour before starting the prefetch for the HFiles that are already scheduled?
Would it be possible to "reset" the delay for the waiting prefetch tasks?
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java:
##########
@@ -298,6 +299,44 @@ public void testPrefetchDoesntSkipRefs() throws Exception {
});
}
+ @Test
+ public void testOnConfigurationChange() {
+ // change PREFETCH_DELAY_ENABLE_KEY from false to true
Review Comment:
I don't see this constant.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestPrefetch.java:
##########
@@ -298,6 +299,44 @@ public void testPrefetchDoesntSkipRefs() throws Exception {
});
}
+ @Test
+ public void testOnConfigurationChange() {
+ // change PREFETCH_DELAY_ENABLE_KEY from false to true
+ conf.setInt(PREFETCH_DELAY, 40000);
+ PrefetchExecutor.loadConfiguration(conf);
Review Comment:
Can't you use `onConfigurationChange` instead?
--
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]
