simbadzina commented on code in PR #6774:
URL: https://github.com/apache/hadoop/pull/6774#discussion_r1597092684
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/ValueQueue.java:
##########
@@ -269,12 +269,23 @@ public ValueQueue(final int numValues, final float
lowWaterMark, long expiry,
* Initializes the Value Queues for the provided keys by calling the
* fill Method with "numInitValues" values
* @param keyNames Array of key Names
- * @throws ExecutionException executionException.
+ * @throws IOException if no successful initialization for any key
Review Comment:
The wording here is confusing. One way to read this is if any key. fails to
initialize, then an except will be thrown. But IIUC an exception will be thrown
if all keys fail to initialize.
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirEncryptionZoneOp.java:
##########
@@ -537,12 +537,12 @@ static boolean isInAnEZ(final FSDirectory fsd, final
INodesInPath iip)
* then launch up a separate thread to warm them up.
*/
static void warmUpEdekCache(final ExecutorService executor,
- final FSDirectory fsd, final int delay, final int interval) {
+ final FSDirectory fsd, final int delay, final int interval, final int
maxRetries) {
Review Comment:
Can you edit a comment in the function documentation to indicate that the
warm up is best effort.
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirEncryptionZoneOp.java:
##########
@@ -580,15 +583,15 @@ public void run() {
final int logCoolDown = 10000; // periodically print error log (if any)
int sinceLastLog = logCoolDown; // always print the first failure
boolean success = false;
+ int retryCount = 0;
IOException lastSeenIOE = null;
long warmUpEDEKStartTime = monotonicNow();
- while (true) {
+
+ while (!success && retryCount < maxRetries) {
try {
kp.warmUpEncryptedKeys(keyNames);
- NameNode.LOG
- .info("Successfully warmed up {} EDEKs.", keyNames.length);
+ NameNode.LOG.info("Successfully warmed up {} EDEKs.",
keyNames.length);
success = true;
- break;
} catch (IOException ioe) {
lastSeenIOE = ioe;
if (sinceLastLog >= logCoolDown) {
Review Comment:
`sinceLastLog` is no longer really used now. You can just print the failure
since the retry count is limited.
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSDirEncryptionZoneOp.java:
##########
@@ -0,0 +1,59 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdfs.server.namenode;
+
+import java.io.IOException;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.crypto.key.KeyProviderCryptoExtension;
+import org.apache.hadoop.hdfs.server.common.HdfsServerConstants.NamenodeRole;
+
+import org.junit.Test;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+public class TestFSDirEncryptionZoneOp {
+
+ @Test
+ public void testWarmUpEdekCacheRetries() throws IOException {
+ NameNode.initMetrics(new Configuration(), NamenodeRole.NAMENODE);
+
+ final int initialDelay = 100;
+ final int retryInterval = 100;
+ final int maxRetries = 2;
+
+ KeyProviderCryptoExtension kpMock = mock(KeyProviderCryptoExtension.class);
+
+ doThrow(new IOException())
+ .doThrow(new IOException())
+ .doAnswer(invocation -> null)
+ .when(kpMock).warmUpEncryptedKeys(any());
+
+ FSDirEncryptionZoneOp.EDEKCacheLoader loader =
+ new FSDirEncryptionZoneOp.EDEKCacheLoader(new String[] {"edek1",
"edek2"}, kpMock,
+ initialDelay, retryInterval, maxRetries);
+
+ loader.run();
+
+ verify(kpMock, times(maxRetries)).warmUpEncryptedKeys(any());
+ }
+}
Review Comment:
Can you add a test cache in which one or some of the keys are successfully
warmed up, while others aren't.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/ValueQueue.java:
##########
@@ -269,12 +269,23 @@ public ValueQueue(final int numValues, final float
lowWaterMark, long expiry,
* Initializes the Value Queues for the provided keys by calling the
* fill Method with "numInitValues" values
* @param keyNames Array of key Names
- * @throws ExecutionException executionException.
+ * @throws IOException if no successful initialization for any key
*/
- public void initializeQueuesForKeys(String... keyNames)
- throws ExecutionException {
+ public void initializeQueuesForKeys(String... keyNames) throws IOException {
+ int successfulInitializations = 0;
+ ExecutionException lastException = null;
+
for (String keyName : keyNames) {
- keyQueues.get(keyName);
+ try {
+ keyQueues.get(keyName);
+ successfulInitializations++;
+ } catch (ExecutionException e) {
+ lastException = e;
+ }
+ }
+
+ if (keyNames.length > 0 && successfulInitializations == 0) {
+ throw new IOException("Failed to initialize any queue for the provided
keys.", lastException);
Review Comment:
It seems you've made warm up a best error operation. If so, there should be
no need to through an exception here. Just logging a warning should be enough.
--
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]
