xkrogen commented on code in PR #4209:
URL: https://github.com/apache/hadoop/pull/4209#discussion_r1033958544
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/qjournal/server/JournaledEditsCache.java:
##########
@@ -277,11 +279,10 @@ void storeEdits(byte[] inputData, long newStartTxn, long
newEndTxn,
initialize(INVALID_TXN_ID);
Journal.LOG.warn(String.format("A single batch of edits was too " +
"large to fit into the cache: startTxn = %d, endTxn = %d, " +
- "input length = %d. The capacity of the cache (%s) must be " +
+ "input length = %d. The capacity of the cache must be " +
Review Comment:
Can we keep the key here in the error message, but print both? Like `(%s or
%s)` where one is `DFS_JOURNALNODE_EDIT_CACHE_SIZE_KEY` and one is
`DFS_JOURNALNODE_EDIT_CACHE_SIZE_FRACTION_KEY`
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/qjournal/server/TestJournaledEditsCache.java:
##########
@@ -221,6 +224,24 @@ public void testCacheMalformedInput() throws Exception {
cache.retrieveEdits(-1, 10, new ArrayList<>());
}
+ @Test
+ public void testCacheFraction() {
+ // Set dfs.journalnode.edit-cache-size.bytes.
+ Configuration config = new Configuration();
+ config.setInt(DFSConfigKeys.DFS_JOURNALNODE_EDIT_CACHE_SIZE_KEY, 1);
+
config.setFloat(DFSConfigKeys.DFS_JOURNALNODE_EDIT_CACHE_SIZE_FRACTION_KEY,
0.1f);
+ cache = new JournaledEditsCache(config);
+ assertEquals(1, cache.getCapacity(), 0.0);
Review Comment:
why do we do this as a floating-point comparison? can't we just do
`assertEquals(1, cache.getCapacity())`
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/qjournal/server/JournaledEditsCache.java:
##########
@@ -121,12 +121,14 @@ class JournaledEditsCache {
// ** End lock-protected fields **
JournaledEditsCache(Configuration conf) {
+ float fraction =
conf.getFloat(DFSConfigKeys.DFS_JOURNALNODE_EDIT_CACHE_SIZE_FRACTION_KEY,
+ DFSConfigKeys.DFS_JOURNALNODE_EDIT_CACHE_SIZE_FRACTION_DEFAULT);
Review Comment:
maybe enforce a check here to guarantee that `fraction < 1.0` ? to fail-fast
in case of misconfigurations
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/qjournal/server/TestJournaledEditsCache.java:
##########
@@ -221,6 +224,24 @@ public void testCacheMalformedInput() throws Exception {
cache.retrieveEdits(-1, 10, new ArrayList<>());
}
+ @Test
+ public void testCacheFraction() {
Review Comment:
how about `testCacheSizeConfigs` ? since we are testing both of them and how
they interact
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/qjournal/server/TestJournaledEditsCache.java:
##########
@@ -221,6 +224,24 @@ public void testCacheMalformedInput() throws Exception {
cache.retrieveEdits(-1, 10, new ArrayList<>());
}
+ @Test
+ public void testCacheFraction() {
+ // Set dfs.journalnode.edit-cache-size.bytes.
+ Configuration config = new Configuration();
+ config.setInt(DFSConfigKeys.DFS_JOURNALNODE_EDIT_CACHE_SIZE_KEY, 1);
+
config.setFloat(DFSConfigKeys.DFS_JOURNALNODE_EDIT_CACHE_SIZE_FRACTION_KEY,
0.1f);
+ cache = new JournaledEditsCache(config);
+ assertEquals(1, cache.getCapacity(), 0.0);
+
+ // Don't set dfs.journalnode.edit-cache-size.bytes.
+ Configuration config1 = new Configuration();
+
config1.setFloat(DFSConfigKeys.DFS_JOURNALNODE_EDIT_CACHE_SIZE_FRACTION_KEY,
0.1f);
+ cache = new JournaledEditsCache(config1);
+ assertEquals(
+ memory *
config1.getFloat(DFSConfigKeys.DFS_JOURNALNODE_EDIT_CACHE_SIZE_FRACTION_KEY,
0.0f),
+ cache.getCapacity(), 0.0);
Review Comment:
how about just this?
```suggestion
assertEquals(memory / 10, cache.getCapacity());
```
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml:
##########
@@ -4955,6 +4955,19 @@
</description>
</property>
+<property>
+ <name>dfs.journalnode.edit-cache-size.fraction</name>
+ <value>0.5f</value>
+ <description>
+ This ratio refers to the proportion of the maximum memory of the JVM.
+ Used to calculate the size of the edits cache that is kept in the
JournalNode's memory.
Review Comment:
we should explicitly mention that this is an alternative to the `.bytes`
config and either copy some of the guidance there (about when it will be
enabled, txn size, etc.) or reference it like "see the documentation for
`dfs.journalnode.edit-cache-size.bytes` to learn more"
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/qjournal/server/TestJournaledEditsCache.java:
##########
@@ -221,6 +224,24 @@ public void testCacheMalformedInput() throws Exception {
cache.retrieveEdits(-1, 10, new ArrayList<>());
}
+ @Test
+ public void testCacheFraction() {
+ // Set dfs.journalnode.edit-cache-size.bytes.
+ Configuration config = new Configuration();
+ config.setInt(DFSConfigKeys.DFS_JOURNALNODE_EDIT_CACHE_SIZE_KEY, 1);
+
config.setFloat(DFSConfigKeys.DFS_JOURNALNODE_EDIT_CACHE_SIZE_FRACTION_KEY,
0.1f);
+ cache = new JournaledEditsCache(config);
+ assertEquals(1, cache.getCapacity(), 0.0);
+
+ // Don't set dfs.journalnode.edit-cache-size.bytes.
+ Configuration config1 = new Configuration();
+
config1.setFloat(DFSConfigKeys.DFS_JOURNALNODE_EDIT_CACHE_SIZE_FRACTION_KEY,
0.1f);
+ cache = new JournaledEditsCache(config1);
+ assertEquals(
+ memory *
config1.getFloat(DFSConfigKeys.DFS_JOURNALNODE_EDIT_CACHE_SIZE_FRACTION_KEY,
0.0f),
+ cache.getCapacity(), 0.0);
+ }
Review Comment:
can we also confirm the default behavior (neither config set) ?
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml:
##########
@@ -4955,6 +4955,19 @@
</description>
</property>
+<property>
+ <name>dfs.journalnode.edit-cache-size.fraction</name>
+ <value>0.5f</value>
+ <description>
+ This ratio refers to the proportion of the maximum memory of the JVM.
+ Used to calculate the size of the edits cache that is kept in the
JournalNode's memory.
+ The recommended value is less than 0.9. We recommend using
+ dfs.journalnode.edit-cache-size.fraction instead of
+ dfs.journalnode.edit-cache-size.bytes. If we set
+ dfs.journalnode.edit-cache-size.bytes, this parameter will not take effect.
Review Comment:
I don't think we specifically recommend one or the other? They are just
different ways to configure it.
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml:
##########
@@ -4955,6 +4955,19 @@
</description>
Review Comment:
for `dfs.journalnode.edit-cache-size.bytes`, we also need to update the
documentation:
1. remove the default value, since there is none. mention that the size
defaults to the value of the `.fraction` config
2. mention that it takes precedence over the `.fraction` config, so if both
are set, then `.bytes` is used
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/qjournal/server/TestJournaledEditsCache.java:
##########
@@ -56,8 +56,11 @@ public class TestJournaledEditsCache {
PathUtils.getTestDir(TestJournaledEditsCache.class, false);
private JournaledEditsCache cache;
+ private long memory;
+
Review Comment:
this field seems kind of unnecessary -- can we just call
`Runtime.getRuntime().maxMemory()` directly on L241? it should at least be a
local field
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
