Copilot commented on code in PR #10156:
URL: https://github.com/apache/ozone/pull/10156#discussion_r3165208729
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFsSnapshot.java:
##########
@@ -212,6 +225,44 @@ void testCreateSnapshotSuccess(String snapshotName)
assertNotNull(snapshotInfo);
}
+ @Test
+ void testSnapshotRenameBlockedWhenConfigDisallows(@TempDir Path tempDir)
+ throws Exception {
+ assertFalse(cluster.getConf().getBoolean(
+ OMConfigKeys.OZONE_OM_SNAPSHOT_RENAME_ALLOWED_KEY,
+ OMConfigKeys.OZONE_OM_SNAPSHOT_RENAME_ALLOWED_DEFAULT));
+
+ String oldSnapshotName = createSnapshot();
+ String newSnapshotName = "snap-" + counter.incrementAndGet();
+
+ try (OzoneClient ozoneClient = cluster.newClient()) {
+ OMException omException = assertThrows(OMException.class,
+ () -> ozoneClient.getObjectStore().renameSnapshot(
+ VOLUME, BUCKET, oldSnapshotName, newSnapshotName));
+ assertEquals(FEATURE_NOT_ENABLED, omException.getResult());
+ assertEquals(SNAPSHOT_RENAME_NOT_ALLOWED_MESSAGE,
+ omException.getMessage());
Review Comment:
This test currently requires the OMException message to match exactly. To
make the test less fragile to future message improvements (eg including config
key/value or restart guidance), prefer asserting the result code and that the
message contains a stable substring such as the config key
(`ozone.om.snapshot.rename.allowed`).
```suggestion
assertThat(omException.getMessage()).contains(
OMConfigKeys.OZONE_OM_SNAPSHOT_RENAME_ALLOWED_KEY);
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotRenameRequest.java:
##########
@@ -69,6 +72,13 @@ public OMSnapshotRenameRequest(OMRequest omRequest) {
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
final OMRequest omRequest = super.preExecute(ozoneManager);
+ if (!ozoneManager.getConfiguration().getBoolean(
+ OZONE_OM_SNAPSHOT_RENAME_ALLOWED_KEY,
+ OZONE_OM_SNAPSHOT_RENAME_ALLOWED_DEFAULT)) {
+ throw new OMException("Ozone snapshot rename feature is not allowed per
Ozone Manager server config",
Review Comment:
The OMException message is not very actionable for operators because it
doesn’t mention the specific config key/value needed to enable the feature.
Consider including `ozone.om.snapshot.rename.allowed=true` (and any restart
requirement, if applicable) in the message so CLI/client users can self-serve.
```suggestion
throw new OMException("Ozone snapshot rename feature is disabled. "
+ "Enable it by setting " + OZONE_OM_SNAPSHOT_RENAME_ALLOWED_KEY
+ "=true in the Ozone Manager server configuration.",
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotRenameRequest.java:
##########
@@ -87,6 +92,21 @@ public void testPreExecute(String toSnapshotName) throws
Exception {
doPreExecute(omRequest);
}
+ @Test
+ public void testPreExecuteFailsWhenSnapshotRenameNotAllowed() {
+ assertFalse(OMConfigKeys.OZONE_OM_SNAPSHOT_RENAME_ALLOWED_DEFAULT);
+ getOzoneManager().getConfiguration().unset(
+ OMConfigKeys.OZONE_OM_SNAPSHOT_RENAME_ALLOWED_KEY);
+
+ OzoneManagerProtocolProtos.OMRequest omRequest = renameSnapshotRequest(
+ getVolumeName(), getBucketName(), snapshotName1, snapshotName2);
+ OMException omException = assertThrows(OMException.class,
+ () -> doPreExecute(omRequest));
+ assertEquals(FEATURE_NOT_ENABLED, omException.getResult());
+ assertEquals("Ozone snapshot rename feature is not allowed per Ozone
Manager server config",
+ omException.getMessage());
Review Comment:
This test asserts the exact exception message string. Since the message is
not a stable API contract and may reasonably change (eg to include the config
key/value), consider asserting on the result code plus `contains`/`startsWith`
for the key part of the message instead of full equality to reduce brittleness.
```suggestion
assertTrue(omException.getMessage().contains(
"Ozone snapshot rename feature is not allowed"));
```
--
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]
