rich7420 commented on code in PR #11252:
URL: https://github.com/apache/ozone/pull/11252#discussion_r4141412819
##########
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java:
##########
@@ -300,6 +306,48 @@ public void testPutObject() {
assertEquals("\"37b51d194a7513e45b56f6524f2d51f2\"",
getObjectResponse.eTag());
}
+ @ParameterizedTest
+ @ValueSource(strings = {"follower-stale", "follower-linearizable",
+ "leader-only"})
+ public void testGetObjectWithReadConsistencyHeader(String readConsistency) {
Review Comment:
S3 authentication still requires the leader before read dispatch, for both
ordinary credentials and STS. I confirmed that both paths reject follower-read
hints. Please add a secure HA test that verifies a follower serves the read;
the current single-OM tests cannot distinguish this from leader fallback.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java:
##########
@@ -206,20 +208,29 @@ public void start() throws IOException {
@Override
public OMResponse submitRequest(OMRequest payload) throws IOException {
- if (useFollowerRead && OmUtils.shouldSendToFollower(payload)) {
+ if (shouldUseFollowerRead(payload)) {
return submitRequestWithFollowerRead(payload);
}
return submitRequestToLeader(addReadConsistencyHint(payload,
leaderReadConsistency));
}
+ private boolean shouldUseFollowerRead(OMRequest payload) {
+ if (!omServiceSupportsFollowerRead ||
!OmUtils.shouldSendToFollower(payload)) {
+ return false;
+ }
+ return defaultFollowerReadEnabled || payload.hasReadConsistencyHint()
+ && ReadConsistency.fromProto(payload.getReadConsistencyHint()
+ .getReadConsistency()).allowFollowerRead();
+ }
+
private OMResponse submitRequestWithFollowerRead(OMRequest payload)
throws IOException {
OMRequest followerPayload = addReadConsistencyHint(payload,
followerReadConsistency);
int failedCount = 0;
- for (int i = 0; useFollowerRead &&
- i < omFailoverProxyProvider.getOMProxyMap().getNodeIds().size(); i++) {
+ for (int i = 0;
+ i < omFailoverProxyProvider.getOMProxyMap().getNodeIds().size(); i++)
{
String nodeId = getCurrentFollowerReadNodeId();
Review Comment:
Please apply the same known-leader avoidance as Hadoop RPC for
`LOCAL_LEASE`. In the gRPC test, `follower-stale` stays on the known leader
even with another OM available, contrary to the routing table in the PR
description.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java:
##########
@@ -206,20 +208,29 @@ public void start() throws IOException {
@Override
public OMResponse submitRequest(OMRequest payload) throws IOException {
- if (useFollowerRead && OmUtils.shouldSendToFollower(payload)) {
+ if (shouldUseFollowerRead(payload)) {
return submitRequestWithFollowerRead(payload);
}
return submitRequestToLeader(addReadConsistencyHint(payload,
leaderReadConsistency));
}
+ private boolean shouldUseFollowerRead(OMRequest payload) {
+ if (!omServiceSupportsFollowerRead ||
!OmUtils.shouldSendToFollower(payload)) {
+ return false;
+ }
+ return defaultFollowerReadEnabled || payload.hasReadConsistencyHint()
Review Comment:
Please give the explicit hint precedence over `defaultFollowerReadEnabled`.
With the default enabled, `leader-only` reaches a follower, and its rejection
disables subsequent follower reads. Both cases reproduced locally and pass with
hint-first evaluation. Can we share this decision with Hadoop RPC to keep the
two transports consistent?
--
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]
