Copilot commented on code in PR #11444:
URL: https://github.com/apache/ozone/pull/11444#discussion_r4216820192
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -3007,6 +3007,11 @@ public void setTimes(OzoneObj obj, String keyName, long
mtime, long atime)
ozoneManagerClient.setTimes(builder.build(), mtime, atime);
}
+ @Override
+ public void setVerifyChecksum(boolean verifyChecksum) {
+ clientConfig.setChecksumVerify(verifyChecksum);
+ }
Review Comment:
`setVerifyChecksum` mutates `clientConfig`, which is likely read
concurrently by threads opening read streams. If the underlying
`OzoneClientConfig` field isn't `volatile` / atomic (or otherwise safely
published), this introduces a data race and the updated value may not be
visible across threads. Consider making the verify-checksum setting thread-safe
(e.g., store it in an `AtomicBoolean`/`volatile` field that streams consult
when created, synchronize updates, or otherwise ensure safe publication).
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java:
##########
@@ -725,6 +725,11 @@ workingDir, getUsername(), true)
return statuses.toArray(new FileStatus[0]);
}
+ @Override
+ public void setVerifyChecksum(boolean verifyChecksum) {
Review Comment:
`FileSystem` has built-in `setVerifyChecksum/getVerifyChecksum` state.
Overriding `setVerifyChecksum` without delegating to
`super.setVerifyChecksum(verifyChecksum)` can leave the base `FileSystem` state
inconsistent (e.g., `getVerifyChecksum()` returning the old value, or other
`FileSystem` logic consulting the field). Consider calling
`super.setVerifyChecksum(verifyChecksum)` in addition to forwarding to
`adapter` to preserve expected `FileSystem` semantics.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java:
##########
@@ -1639,6 +1639,14 @@ ListSnapshotDiffJobResponse listSnapshotDiffJobs(
void setTimes(OzoneObj obj, String keyName, long mtime, long atime)
throws IOException;
+ /**
+ * Set whether this client verifies checksums when reading. Overrides
ozone.client.verify.checksum for this client
+ * only. Takes effect for input streams opened afterwards, streams that are
already open may or may not pick it up.
+ * @param verifyChecksum false to skip checksum verification, e.g. when the
application verifies its own checksums.
+ */
+ default void setVerifyChecksum(boolean verifyChecksum) {
+ }
Review Comment:
The Javadoc lines exceed typical checkstyle limits (often 100–120 chars) and
may fail style checks in some builds. Consider wrapping the prose and the
`@param` description onto multiple shorter lines to match the project's
formatting conventions.
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java:
##########
@@ -978,6 +978,11 @@ private List<FileStatusAdapter> listStatusAdapter(Path f,
boolean lite) throws I
return statuses;
}
+ @Override
+ public void setVerifyChecksum(boolean verifyChecksum) {
Review Comment:
Same issue as in `BasicOzoneFileSystem`: overriding `setVerifyChecksum`
without updating the base `FileSystem` verifyChecksum field can create
inconsistent behavior for callers that rely on `FileSystem`'s state (including
`getVerifyChecksum()`). Consider calling
`super.setVerifyChecksum(verifyChecksum)` before/after delegating to `adapter`.
--
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]
