Copilot commented on code in PR #10030:
URL: https://github.com/apache/ozone/pull/10030#discussion_r3089158895
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java:
##########
@@ -338,6 +388,75 @@ public void close() throws IOException {
shutdown();
}
+ private OMResponse submitRequest(ManagedChannel channel, OMRequest request,
+ String clientHostname, String clientIpAddress) {
+ Metadata headers = new Metadata();
+ if (clientHostname != null) {
+ headers.put(CLIENT_HOSTNAME_METADATA_KEY, clientHostname);
+ }
+ if (clientIpAddress != null) {
+ headers.put(CLIENT_IP_ADDRESS_METADATA_KEY, clientIpAddress);
+ }
+
+ org.apache.ratis.thirdparty.io.grpc.Channel intercepted =
+ ClientInterceptors.intercept(channel, new
FixedHeadersInterceptor(headers));
+ return ClientCalls.blockingUnaryCall(intercepted, SUBMIT_REQUEST_METHOD,
+ CallOptions.DEFAULT, request);
+ }
+
+ private static final class FixedHeadersInterceptor implements
ClientInterceptor {
+ private final Metadata headers;
+
+ private FixedHeadersInterceptor(Metadata headers) {
+ this.headers = headers;
+ }
+
+ @Override
+ public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
+ MethodDescriptor<ReqT, RespT> methodDescriptor,
+ CallOptions callOptions,
+ org.apache.ratis.thirdparty.io.grpc.Channel channel) {
+ return new ForwardingClientCall.SimpleForwardingClientCall<ReqT, RespT>(
+ channel.newCall(methodDescriptor, callOptions)) {
+ @Override
+ public void start(Listener<RespT> responseListener, Metadata metadata)
{
+ metadata.merge(headers);
+ super.start(responseListener, metadata);
+ }
+ };
+ }
+ }
+
+ private static final class Proto2Marshaller<T> implements
MethodDescriptor.Marshaller<T> {
+ private final Proto2Parser<T> parser;
+
+ private Proto2Marshaller(Proto2Parser<T> parser) {
+ this.parser = parser;
+ }
+
+ @Override
+ public InputStream stream(T value) {
+ if (!(value instanceof com.google.protobuf.MessageLite)) {
+ throw new IllegalArgumentException("Expected protobuf
request/response");
+ }
+ return new ByteArrayInputStream(((com.google.protobuf.MessageLite)
value).toByteArray());
+ }
Review Comment:
Proto2Marshaller.stream() serializes via MessageLite.toByteArray(), which
always copies the message into a new byte[]. This contradicts the PR goal of
preserving zero-copy marshalling and can add avoidable allocations on every
RPC. Prefer streaming from the message's ByteString (eg.
toByteString().newInput()) or another zero-copy InputStream source.
##########
pom.xml:
##########
@@ -2285,6 +2365,15 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-clean-plugin</artifactId>
<configuration>
+ <!--
+ The Cursor/VSCode Java Language Server recreates class files in
target/classes/
+ and target/test-classes/ (with com.apple.provenance extended
attributes) faster
+ than the pre-clean rm -rf can remove them. failOnError=false lets
the build
+ continue even if the clean plugin cannot remove those IDE-managed
files; the
+ subsequent compile phase writes fresh class files over them so the
build result
+ is still correct.
+ -->
+ <failOnError>false</failOnError>
Review Comment:
Setting maven-clean-plugin <failOnError>false</failOnError> globally can
mask legitimate clean failures (eg. permission issues) and allow stale build
outputs to leak into subsequent phases. Consider scoping this workaround to the
affected environment (eg. a macOS-only profile/property) so other platforms
still fail fast on real clean errors.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestDelegationToken.java:
##########
@@ -389,14 +390,19 @@ public void testDelegationToken(boolean useIp) throws
Exception {
// Case 6: Test failure of token cancellation.
// Get Om client, this time authentication using Token will fail as
- // token is not in cache anymore.
+ // token is not in cache anymore. Use minimal retries so the default
+ // failover policy (500 attempts × 2 s) does not exhaust the 5-minute
+ // test timeout before the security rejection propagates. The
+ // rejection may arrive as OMException (OM-level) or a plain IOException
+ // (transport/auth-level), both are valid security rejections.
+ OzoneConfiguration fastConf = new OzoneConfiguration(conf);
+ fastConf.setInt(OZONE_CLIENT_FAILOVER_MAX_ATTEMPTS_KEY, 3);
+ fastConf.setLong(OZONE_CLIENT_WAIT_BETWEEN_RETRIES_MILLIS_KEY, 100L);
omClient = new OzoneManagerProtocolClientSideTranslatorPB(
- OmTransportFactory.create(conf, testUser, null),
+ OmTransportFactory.create(fastConf, testUser, null),
RandomStringUtils.secure().nextAscii(5));
- ex = assertThrows(OMException.class,
+ assertThrows(IOException.class,
() -> omClient.cancelDelegationToken(token));
Review Comment:
This assertion was broadened to any IOException, which weakens the check
that token-cancel fails with the expected security-related error. To keep the
test meaningful while still avoiding long retry timeouts, consider asserting on
the IOException cause/message (or using assertThrowsExactly/expected subclasses
like OMException/AccessControlException) rather than any IOException.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java:
##########
@@ -338,6 +388,75 @@ public void close() throws IOException {
shutdown();
}
+ private OMResponse submitRequest(ManagedChannel channel, OMRequest request,
+ String clientHostname, String clientIpAddress) {
+ Metadata headers = new Metadata();
+ if (clientHostname != null) {
+ headers.put(CLIENT_HOSTNAME_METADATA_KEY, clientHostname);
+ }
+ if (clientIpAddress != null) {
+ headers.put(CLIENT_IP_ADDRESS_METADATA_KEY, clientIpAddress);
+ }
+
+ org.apache.ratis.thirdparty.io.grpc.Channel intercepted =
+ ClientInterceptors.intercept(channel, new
FixedHeadersInterceptor(headers));
+ return ClientCalls.blockingUnaryCall(intercepted, SUBMIT_REQUEST_METHOD,
+ CallOptions.DEFAULT, request);
Review Comment:
submitRequest() wraps the ManagedChannel with
ClientInterceptors.intercept(...) on every call to attach fixed headers. This
creates additional wrapper allocations per RPC. Since the hostname/IP values
are effectively constant for the process, consider attaching a headers
interceptor once at channel construction time (or caching the intercepted
Channel) to reduce per-call overhead.
##########
pom.xml:
##########
@@ -2318,6 +2440,174 @@
</target>
</configuration>
</execution>
+ <execution>
+ <!--
+ When running with -Dmaven.test.skip=true, Maven does not compile
test classes.
+ However the Cursor/VSCode Java Language Server writes class
files into
+ target/test-classes/ concurrently. These IDE-generated files
may be partially
+ written (corrupted) and will cause the maven-dependency-plugin
analyze-only goal
+ (bound to the verify phase) to fail with "Index out of bounds"
class-reading
+ errors. Deleting target/test-classes/ in prepare-package (which
runs just
+ before verify) removes these LSP-generated stubs before the
analyzer runs,
+ making the analysis deterministic and correct.
+ -->
+ <id>pre-verify-delete-test-classes</id>
+ <goals>
+ <goal>run</goal>
+ </goals>
+ <phase>pre-integration-test</phase>
+ <configuration>
+ <target>
+ <exec executable="/bin/rm" failonerror="false">
+ <arg value="-rf" />
+ <arg value="${project.build.directory}/test-classes" />
+ </exec>
+ </target>
+ </configuration>
+ </execution>
+ <execution>
+ <!--
+ Refresh target/classes/ from the freshly-packaged JAR just
before the
+ dependency:analyze-only goal (which runs in the verify phase).
The Cursor/LSP
+ Java Language Server may overwrite class files in
target/classes/ with corrupt
+ or stale copies between the compile and verify phases. By
deleting classes/
+ and re-extracting from the module JAR (produced by
maven-jar-plugin in the
+ package phase), we guarantee that the dependency analyzer sees
clean, correct
+ bytecode. For pom-packaging modules with no JAR, the unzip exec
fails
+ silently (failonerror=false) and classes/ remains empty - which
is correct.
+ -->
+ <id>pre-verify-refresh-classes-from-jar</id>
+ <goals>
+ <goal>run</goal>
+ </goals>
+ <phase>pre-integration-test</phase>
+ <configuration>
+ <target>
+ <!--
+ Only wipe and restore target/classes/ when the module JAR
was actually produced
+ (i.e. this is a jar-packaging module whose package phase ran
successfully).
+ For pom-packaging modules – and for any edge case where the
JAR is absent –
+ we skip the destructive rm -rf so that the freshly compiled
class files are
+ left undisturbed for downstream reactor modules to compile
against.
+ A POSIX shell one-liner is used because the Ant bundled with
+ maven-antrun-plugin does not support if/unless on <exec>.
+ -->
+ <exec executable="/bin/sh" failonerror="false">
+ <arg value="-c" />
+ <arg
value="JAR='${project.build.directory}/${project.build.finalName}.jar'; test -f
"$JAR" && /bin/rm -rf '${project.build.outputDirectory}'
&& unzip -qo "$JAR" -d '${project.build.outputDirectory}'" />
+ </exec>
Review Comment:
The pre-verify-refresh-classes-from-jar execution relies on /bin/sh and
unzip being available and uses a shell one-liner to conditionally
delete/restore target/classes. This is fragile and non-portable; consider using
Ant condition + built-in <delete>/<unzip> tasks (or a platform-scoped profile)
so the behavior is deterministic across environments.
##########
pom.xml:
##########
@@ -2305,6 +2394,39 @@
<artifactId>maven-antrun-plugin</artifactId>
<version>${maven-antrun-plugin.version}</version>
<executions>
+ <execution>
+ <!--
+ On macOS the Cursor/LSP Java Language Server writes class files
with the
+ com.apple.provenance extended attribute into target/classes/.
Java NIO's
+ Files.delete() (used by maven-clean-plugin) cannot remove those
entries,
+ aborting the clean and leaving a mixed-state target directory
that confuses
+ incremental compilation and the dependency analyzer. Deleting
via the OS
+ rm command in the pre-clean phase resolves this: by the time
maven-clean-plugin
+ runs, the target directory no longer exists so the plugin has
nothing to do.
+ -->
+ <id>pre-clean-force-delete-target</id>
+ <goals>
+ <goal>run</goal>
+ </goals>
+ <phase>pre-clean</phase>
+ <configuration>
+ <target>
+ <!--
+ On macOS, the Cursor/LSP Java Language Server writes class
files with the
+ com.apple.provenance extended attribute into
target/classes/. This attribute
+ is a protected system attribute that cannot be removed by
xattr -cr; Java
+ NIO's Files.delete() (used by maven-clean-plugin) also
cannot delete such
+ files. Using /bin/rm -rf in the pre-clean phase deletes the
entire target
+ directory via the unlink() syscall, which is not subject to
this restriction.
+ By the time maven-clean-plugin runs, the target directory no
longer exists.
+ -->
+ <exec executable="/bin/rm" failonerror="false">
+ <arg value="-rf" />
+ <arg value="${project.build.directory}" />
+ </exec>
Review Comment:
The build now executes /bin/rm during pre-clean. This is non-portable (eg.
Windows) and also bypasses Maven's normal clean semantics. Please gate this
execution behind an OS-activated profile (macOS only) and/or an explicit opt-in
property so CI and other developer environments are not affected.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java:
##########
@@ -72,22 +88,36 @@ public class GrpcOmTransport implements OmTransport {
LoggerFactory.getLogger(GrpcOmTransport.class);
private static final String CLIENT_NAME = "GrpcOmTransport";
+ private static final String SERVICE_NAME =
"hadoop.ozone.OzoneManagerService";
private static final int SHUTDOWN_WAIT_INTERVAL = 100;
private static final int SHUTDOWN_MAX_WAIT_SECONDS = 5;
private final AtomicBoolean isRunning = new AtomicBoolean(false);
// gRPC specific
private static List<X509Certificate> caCerts = null;
- private final Map<String,
- OzoneManagerServiceGrpc.OzoneManagerServiceBlockingStub> clients;
+ private static final Metadata.Key<String> CLIENT_HOSTNAME_METADATA_KEY =
+ Metadata.Key.of("CLIENT_HOSTNAME", Metadata.ASCII_STRING_MARSHALLER);
+ private static final Metadata.Key<String> CLIENT_IP_ADDRESS_METADATA_KEY =
+ Metadata.Key.of("CLIENT_IP_ADDRESS", Metadata.ASCII_STRING_MARSHALLER);
Review Comment:
GrpcOmTransport duplicates the metadata key definitions for
CLIENT_HOSTNAME/CLIENT_IP_ADDRESS instead of reusing GrpcClientConstants. This
risks subtle mismatches if the header names/marshallers change elsewhere.
Prefer referencing GrpcClientConstants.CLIENT_*_METADATA_KEY directly.
```suggestion
GrpcClientConstants.CLIENT_HOSTNAME_METADATA_KEY;
private static final Metadata.Key<String> CLIENT_IP_ADDRESS_METADATA_KEY =
GrpcClientConstants.CLIENT_IP_ADDRESS_METADATA_KEY;
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestSecureOzoneRpcClient.java:
##########
@@ -435,18 +434,29 @@ public void testS3Auth() throws Exception {
}
@Test
- public void testRemoteException() {
+ public void testRemoteException() throws IOException {
UserGroupInformation realUser =
UserGroupInformation.createRemoteUser("realUser");
UserGroupInformation proxyUser =
UserGroupInformation.createProxyUser("user", realUser);
- assertThrows(AccessControlException.class, () -> {
- proxyUser.doAs((PrivilegedExceptionAction<Void>) () -> {
- try (OzoneClient ozoneClient =
OzoneClientFactory.getRpcClient(getCluster().getConf())) {
- ozoneClient.getObjectStore().listVolumes("/");
- }
- return null;
- });
- });
+ // Use minimal retries so the test fails fast instead of hitting the
+ // 5-minute @Test timeout. The proxy user has no valid credentials;
+ // the OM rejects it quickly, but the default failover policy
+ // (500 attempts × 2 s) would exhaust the timeout before propagating
+ // the rejection. The rejection may arrive as AccessControlException
+ // (authorization-layer refusal) or as a plain IOException
+ // (transport/authentication-layer refusal), both of which are valid
+ // security rejections.
+ OzoneConfiguration testConf = new
OzoneConfiguration(getCluster().getConf());
+ testConf.setInt(OzoneConfigKeys.OZONE_CLIENT_FAILOVER_MAX_ATTEMPTS_KEY, 3);
+
testConf.setLong(OzoneConfigKeys.OZONE_CLIENT_WAIT_BETWEEN_RETRIES_MILLIS_KEY,
100L);
+
+ assertThrows(IOException.class, () ->
+ proxyUser.doAs((PrivilegedExceptionAction<Void>) () -> {
+ try (OzoneClient ozoneClient =
OzoneClientFactory.getRpcClient(testConf)) {
+ ozoneClient.getObjectStore().listVolumes("/");
+ }
+ return null;
+ }));
Review Comment:
This test now accepts any IOException, which can also include unrelated
failures (eg. transient network/cluster issues) and reduces the signal that the
rejection is specifically auth-related. Consider asserting on the exception
type/cause chain (eg. AccessControlException/OMException) or matching an
expected message to keep the test deterministic.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManagerServiceGrpc.java:
##########
@@ -89,4 +119,27 @@ private static byte[] getClientId() {
return UUIDUtil.randomUUIDBytes();
}
+ private static <T extends MessageLite> MethodDescriptor.Marshaller<T>
proto2Marshaller(
+ Proto2Parser<T> parser) {
+ return new MethodDescriptor.Marshaller<T>() {
+ @Override
+ public InputStream stream(T value) {
+ return new ByteArrayInputStream(value.toByteArray());
+ }
Review Comment:
proto2Marshaller().stream() uses value.toByteArray(), which forces a full
copy of every request/response. If the intent is to keep zero-copy behavior,
prefer streaming from ByteString (eg. value.toByteString().newInput()) to avoid
extra allocations and match the PR description.
--
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]
