Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -1078,14 +1078,11 @@ private void chainFuture(
}

@Override
public String toString() {
return "OffsetFetchRequestState{" +
"requestedPartitions=" + requestedPartitions +
", memberId=" + memberInfo.memberId.orElse("undefined") +
", memberEpoch=" + (memberInfo.memberEpoch.isPresent() ? memberInfo.memberEpoch.get() : "undefined") +
", future=" + future +
", " + toStringBase() +
'}';
public String toStringBase() {
return super.toStringBase() +
", memberInfo=" + memberInfo +

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memberInfo is from super RetriableRequestState. Maybe we should add toStringBase to RetriableRequestState too?

@lianetm lianetm Jun 11, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with moving this up to the RetriableRequestState, and once there, I would also suggest we simplify the output to show the member id and epoch directly, something like:

Suggested change
", memberInfo=" + memberInfo +
", memberId=" + memberInfo.memberId.orElse... +
", memberEpoch=" + memberInfo.memberEpoch... +

MemberInfo is just an internal wrapper to move the 2 together, but when seeing a toString for an OffsetFetch or OffsetCommit request, we just care about memberId and memberEpoch (they will be included in the request to the broker)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have implemented both suggestions

", requestedPartitions=" + requestedPartitions +
", future=" + future;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure whether it is worth printing future since it show reference address only :(

}
}

Expand Down Expand Up @@ -1278,5 +1275,12 @@ static class MemberInfo {
this.memberId = Optional.empty();
this.memberEpoch = Optional.empty();
}

@Override
public String toString() {
return "MemberInfo{" + "memberId=" + memberId.orElse("undefined") +
", memberEpoch=" + (memberEpoch.isPresent() ? memberEpoch : "undefined") + "}";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memberEpoch.get?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say we need to check if present. There could be no epoch/id when committing/fetching (if we haven't joined a group or left it. In those cases we would have empty here, so that no epoch/id is included in the OffsetFetch/OffsetCommit request).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! And my point was "we should call get if it is present". Otherwise, the toString will include "Optional"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented this suggestion, I will note that when testing the method, there is no difference in how the string is printed whether using memberEpoch.get() or not

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! And my point was "we should call get if it is present". Otherwise, the toString will include "Optional"

Oh I misread your point he he. But both on the same page in the end, great.

}

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
import static org.apache.kafka.clients.consumer.internals.ConsumerTestBuilder.DEFAULT_GROUP_ID;
import static org.apache.kafka.clients.consumer.internals.ConsumerTestBuilder.DEFAULT_GROUP_INSTANCE_ID;
import static org.apache.kafka.test.TestUtils.assertFutureThrows;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
Expand Down Expand Up @@ -122,6 +123,50 @@ public void setup() {
this.props.put(VALUE_DESERIALIZER_CLASS_CONFIG, StringDeserializer.class);
}

@Test
public void testOffsetFetchRequestStateToStringBase() {
ConsumerConfig config = mock(ConsumerConfig.class);
CommitRequestManager.MemberInfo memberInfo = new CommitRequestManager.MemberInfo();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we had discussion about "Optional#toString", maybe we define either MemberInfo#memberId or MemberInfo#memberEpoch, and then we should check the MemberInfo#toString does not contain "Optional[value]"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you meant now, I have implemented a setter for MemberInfo#memberEpoch and set the memberEpoch value. Upon testing, there is no "Optional[value]"

@lianetm lianetm Jun 11, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brenden20 we already have an onMemberEpochUpdated that you could maybe use for this? so we don't have to add a new setter. That's the actual method that gets called when the member gets a new epoch on the heartbeat response, and we want to pass it on to the commit manager ;)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lianetm oh I see it now, I will revert that change and use onMemberEpochUpdated instead. Thank you!


CommitRequestManager commitRequestManager = new CommitRequestManager(
time,
logContext,
subscriptionState,
config,
coordinatorRequestManager,
offsetCommitCallbackInvoker,
"groupId",
Optional.of("groupInstanceId"),
metrics);

Set<TopicPartition> requestedPartitions = new HashSet<>();
TopicPartition topicPartition1 = new TopicPartition("topic-1", 1);
requestedPartitions.add(topicPartition1);
Comment thread
brenden20 marked this conversation as resolved.
Outdated

CommitRequestManager.OffsetFetchRequestState offsetFetchRequestState = commitRequestManager.new OffsetFetchRequestState(
requestedPartitions,
retryBackoffMs,
retryBackoffMaxMs,
1000,
memberInfo);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the line#142 - The memberInfo updated by onMemberEpochUpdated is the inner variable of commitRequestManager. Hence, this memberInfo used to create OffsetFetchRequestState is NOT updated by onMemberEpochUpdated

If we want to complete the test, maybe we can remove MemberInfo from constructor of OffsetFetchRequestState. After all, both RetriableRequestState and OffsetFetchRequestState are the inner class of CommitRequestManager, so it is valid to share the MemberInfo between those classes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit hesitant to change the constructor of OffsetFetchRequestState as I do not want to mess up usages elsewhere in the codebase. I did however find a way to achieve updating the inner memberInfo. Test is working as well, I just needed to change the method from private to protected. Let me know what you think


TimedRequestState timedRequestState = new TimedRequestState(
logContext,
"CommitRequestManager",
retryBackoffMs,
retryBackoffMaxMs,
TimedRequestState.deadlineTimer(time, 0)
);

String target = timedRequestState.toStringBase() +
", memberInfo=" + memberInfo +
", requestedPartitions=" + offsetFetchRequestState.requestedPartitions +
", future=" + offsetFetchRequestState.future();

assertDoesNotThrow(timedRequestState::toString);
assertEquals(target, offsetFetchRequestState.toStringBase());
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add assertFalse(target.contains("Optional")); to make sure we unwrap the optional variables?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added!


@Test
public void testPollSkipIfCoordinatorUnknown() {
CommitRequestManager commitRequestManager = create(false, 0);
Expand Down