You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/12/07 21:41:09 UTC
[GitHub] [ozone] errose28 commented on a change in pull request #1664: HDDS-4540. Add a new OM admin operation to submit the OMPrepareRequest.
errose28 commented on a change in pull request #1664:
URL: https://github.com/apache/ozone/pull/1664#discussion_r537797653
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMPrepareRequest.java
##########
@@ -162,9 +167,7 @@ private static void waitForLogIndex(long indexToWaitFor,
&& (ratisTxnIndex >= indexToWaitFor);
}
- if (!success) {
- Thread.sleep(DOUBLE_BUFFER_FLUSH_CHECK_INTERVAL.toMillis());
- }
+ Thread.sleep(flushCheckInterval.toMillis());
Review comment:
By removing the `if (!success)` condition, we will unnecessarily wait an extra `flushCheckInterval` after the OM is prepared. Minor issue, but could save us a few seconds of waiting.
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/admin/om/PrepareSubCommand.java
##########
@@ -0,0 +1,67 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.ozone.admin.om;
+
+import java.util.concurrent.Callable;
+
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.ozone.om.protocol.OzoneManagerProtocol;
+
+import picocli.CommandLine;
+
+/**
+ * Handler of ozone admin om finalizeUpgrade command.
+ */
+@CommandLine.Command(
+ name = "prepare",
+ description = "Prepares Ozone Manager for upgrade/downgrade, by applying " +
+ "all pending transactions, taking a Ratis snapshot at the last txn " +
+ "and purging all logs on each OM instance. The returned txn id " +
+ "corresponds to the last txn in the quorum in which the snapshot is " +
+ "taken.",
+ mixinStandardHelpOptions = true,
+ versionProvider = HddsVersionProvider.class
+)
+public class PrepareSubCommand implements Callable<Void> {
+
+ @CommandLine.ParentCommand
+ private OMAdmin parent;
+
+ @CommandLine.Option(
+ names = {"-id", "--service-id"},
+ description = "Ozone Manager Service ID",
+ required = true
+ )
+ private String omServiceId;
+
+ @CommandLine.Option(
+ names = {"-ft", "--flush-wait-timeout"},
+ description = "Max time to wait for OM Double Buffer flush in seconds.",
Review comment:
I feel like this flag name and description might be too low level for the UI, since the double buffer is more of an implementation detail. For example, the user would not be expected to know whether the double buffer flush is a mandatory part of prepare (it is), or if this is just a convenience and they can just set this to a low number and if the flush doesn't complete in time expect the OM to still prepare. Maybe just `--prepare-timeout` for the flag? We can use the flush timeout terminology within the client, request, and protos.
If the user sets this too low, they will get an error message saying the flush did not complete in time. Then they can retry with a higher value. So this is just a thought and might not be necessary.
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerPrepare.java
##########
@@ -152,11 +131,10 @@ public void testPrepareWithTransactions() throws Exception {
* Checks that third OM received all transactions and is prepared.
* @throws Exception
*/
- // TODO: Fix this test so it passes.
- // @Test
+// @Test
public void testPrepareDownedOM() throws Exception {
// Index of the OM that will be shut down during this test.
- final int shutdownOMIndex = 2;
+ final int shutdownOMIndex = new Random().nextInt(3);
Review comment:
nit: Is there any benefit to using a random index over a fixed one? When observing the test run from logs, we now need to search the messages to determine which OM was taken down on this run (also distinguish the deliberate takedown from a crash or JVM pause induced leader change), instead of having that info beforehand.
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java
##########
@@ -1547,6 +1547,32 @@ public boolean recoverTrash(String volumeName, String bucketName,
return recoverResponse.getResponse();
}
+ @Override
+ public long prepareOzoneManager(long flushWaitTimeout,
+ long flushCheckInterval) throws IOException {
+ Preconditions.checkArgument(flushWaitTimeout > 0,
+ "flushWaitTimeout has to be > zero");
+
+ Preconditions.checkArgument(flushCheckInterval > 0 &&
+ flushCheckInterval < flushWaitTimeout / 2,
+ "flushCheckInterval has to be > zero and < half of " +
+ "flushWaitTimeout to make sense.");
Review comment:
If the user passes in a value for flush wait timeout that is less than the hardcoded flush check interval, they will see this message. This is confusing to them since it indicates the problem is flush check interval, which they have no knowledge of. They must also guess as to what the flush check interval value is to make sure their flush wait timeout is two times that when they try again.
Similar to above, a min value check on the user passed flush wait timeout to make sure it is large enough (or automatically set flush check interval based on the passed value) might be good to add in `PrepareSubCommand`.
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java
##########
@@ -1547,6 +1547,32 @@ public boolean recoverTrash(String volumeName, String bucketName,
return recoverResponse.getResponse();
}
+ @Override
+ public long prepareOzoneManager(long flushWaitTimeout,
+ long flushCheckInterval) throws IOException {
Review comment:
Can we add some identifying information for these time units in this method? We could use `java.time.Duration`, or since they are being passed right into a proto, just adding `Seconds` onto their variable names might be enough.
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -1079,7 +1079,12 @@ message FinalizeUpgradeProgressResponse {
}
message PrepareRequest {
+ required PrepareRequestArgs args = 1;
+}
+message PrepareRequestArgs {
+ optional uint64 flushWaitTimeOut = 1 [default = 300];
+ optional uint64 flushWaitCheckInterval = 2 [default = 5];
Review comment:
Similar to above, can we add `Seconds` on to these variable names to make sure the sender and receiver are on the same page with how these time durations are interpreted?
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/admin/om/PrepareSubCommand.java
##########
@@ -0,0 +1,67 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.ozone.admin.om;
+
+import java.util.concurrent.Callable;
+
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.ozone.om.protocol.OzoneManagerProtocol;
+
+import picocli.CommandLine;
+
+/**
+ * Handler of ozone admin om finalizeUpgrade command.
+ */
+@CommandLine.Command(
+ name = "prepare",
+ description = "Prepares Ozone Manager for upgrade/downgrade, by applying " +
+ "all pending transactions, taking a Ratis snapshot at the last txn " +
+ "and purging all logs on each OM instance. The returned txn id " +
+ "corresponds to the last txn in the quorum in which the snapshot is " +
+ "taken.",
+ mixinStandardHelpOptions = true,
+ versionProvider = HddsVersionProvider.class
+)
+public class PrepareSubCommand implements Callable<Void> {
+
+ @CommandLine.ParentCommand
+ private OMAdmin parent;
+
+ @CommandLine.Option(
+ names = {"-id", "--service-id"},
+ description = "Ozone Manager Service ID",
+ required = true
+ )
+ private String omServiceId;
+
+ @CommandLine.Option(
+ names = {"-ft", "--flush-wait-timeout"},
+ description = "Max time to wait for OM Double Buffer flush in seconds.",
+ defaultValue = "300"
+ )
+ private long flushWaitTime;
+
+ @Override
+ public Void call() throws Exception {
+ OzoneManagerProtocol client = parent.createOmClient(omServiceId);
+ long prepareTxnId = client.prepareOzoneManager(flushWaitTime, 5);
Review comment:
nit: Can we make the `5` a static constant in this class so it is easier to find and update if needed?
##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/admin/om/PrepareSubCommand.java
##########
@@ -0,0 +1,67 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.ozone.admin.om;
+
+import java.util.concurrent.Callable;
+
+import org.apache.hadoop.hdds.cli.HddsVersionProvider;
+import org.apache.hadoop.ozone.om.protocol.OzoneManagerProtocol;
+
+import picocli.CommandLine;
+
+/**
+ * Handler of ozone admin om finalizeUpgrade command.
+ */
+@CommandLine.Command(
+ name = "prepare",
+ description = "Prepares Ozone Manager for upgrade/downgrade, by applying " +
+ "all pending transactions, taking a Ratis snapshot at the last txn " +
+ "and purging all logs on each OM instance. The returned txn id " +
+ "corresponds to the last txn in the quorum in which the snapshot is " +
+ "taken.",
Review comment:
nit: Is *txn* a standard abbreviation for *transaction* that is used in the docs and the user would be expected to recognize? Seems best not to use non-standard abbreviations in user facing documentation.
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerPrepare.java
##########
@@ -112,26 +100,17 @@ public void testPrepareWithTransactions() throws Exception {
// Make sure all OMs have logs from writing data, so we can check that
// they are purged after prepare.
- for (OzoneManager om: cluster.getOzoneManagersList()) {
- LambdaTestUtils.await(timeoutMillis, 1000,
- () -> logFilesPresentInRatisPeer(om));
- }
+ cluster.getOzoneManagersList().forEach(om ->
+ Assert.assertTrue(logFilesPresentInRatisPeer(om)));
- OzoneManager leader = cluster.getOMLeader();
- OMResponse omResponse = submitPrepareRequest(leader.getOmRatisServer());
- // Get the log index of the prepare request.
+ OzoneManagerProtocol ozoneManagerClient =
+ ozClient.getObjectStore().getClientProxy().getOzoneManagerClient();
long prepareRequestLogIndex =
- omResponse.getPrepareResponse().getTxnID();
+ ozoneManagerClient.prepareOzoneManager(300, 5);
Review comment:
nit: Can we make the flush timeout and check interval constants in this class used for all the tests? Right now its fairly clear, but if someone has to come back and fix this test later because its timing out or taking too long, having the time configs easily visible will help.
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java
##########
@@ -1547,6 +1547,32 @@ public boolean recoverTrash(String volumeName, String bucketName,
return recoverResponse.getResponse();
}
+ @Override
+ public long prepareOzoneManager(long flushWaitTimeout,
+ long flushCheckInterval) throws IOException {
+ Preconditions.checkArgument(flushWaitTimeout > 0,
+ "flushWaitTimeout has to be > zero");
Review comment:
Should we add this check in `PrepareSubCommand` as well to give the user a specific error message if they pass a bad value? I'm not sure how the client handles these precondition generated exceptions and presents them to the user.
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerPrepare.java
##########
@@ -217,15 +188,11 @@ public void testPrepareDownedOM() throws Exception {
// Since prepare was the last Ratis transaction, it should have all data
// it missed once it receives the prepare transaction.
cluster.restartOzoneManager(downedOM, true);
- // Wait for other OMs to catch this one up on transactions.
- LambdaTestUtils.await(timeoutMillis, 1000,
- () -> downedOM.getRatisSnapshotIndex() == prepareIndex);
- checkPrepared(downedOM, prepareIndex);
+ LambdaTestUtils.await(timeoutMillis, 2000,
+ () -> checkPrepared(downedOM, prepareIndex));
Review comment:
Can we replace this with a `waitAndCheckPrepared` call? Then we know that the logs have also been removed as well.
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerPrepare.java
##########
@@ -266,61 +233,23 @@ private void writeTestData(ObjectStore store, String volumeName,
keyStream.close();
}
- private OMRequest buildPrepareRequest() {
- PrepareRequest requestProto = PrepareRequest.newBuilder().build();
-
- return OMRequest.newBuilder()
- .setPrepareRequest(requestProto)
- .setCmdType(Type.Prepare)
- .setClientId(UUID.randomUUID().toString())
- .build();
- }
-
private void waitAndCheckPrepared(OzoneManager om,
long prepareRequestLogIndex) throws Exception {
// Log files are deleted after the snapshot is taken,
// So once log files have been deleted, OM should be prepared.
LambdaTestUtils.await(timeoutMillis, 1000,
() -> !logFilesPresentInRatisPeer(om));
- checkPrepared(om, prepareRequestLogIndex);
+ Assert.assertTrue(checkPrepared(om, prepareRequestLogIndex));
}
- private void checkPrepared(OzoneManager om, long prepareRequestLogIndex)
+ private boolean checkPrepared(OzoneManager om, long prepareRequestLogIndex)
Review comment:
Do we still need this method? Since we are no longer immediately checking the leader and waiting on all 3 OMs, can we just put these lines in `waitAndCheckPrepared`? This method on its own is kind of misleading, since it no longer checks that the logs have been removed, and therefore doesn't do a full check for preparedness.
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerPrepare.java
##########
@@ -112,26 +100,17 @@ public void testPrepareWithTransactions() throws Exception {
// Make sure all OMs have logs from writing data, so we can check that
// they are purged after prepare.
- for (OzoneManager om: cluster.getOzoneManagersList()) {
- LambdaTestUtils.await(timeoutMillis, 1000,
- () -> logFilesPresentInRatisPeer(om));
- }
+ cluster.getOzoneManagersList().forEach(om ->
+ Assert.assertTrue(logFilesPresentInRatisPeer(om)));
Review comment:
This could fail in a slow cluster if the above keys haven't made it to every OM's log yet. Probably unlikely, but if we leave the `await` call in, we reduce some potential flakiness with this test without sacrificing its guarantees.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org