Addressing: #7897 (comment)

Addressing #7897 (comment)

This and the following new tests are addressing the comments for separating valid and invalid scenario: #7897 (comment)

We unify the API for groupId and groupMetadata commit here, which includes the groupId within the metadata struct.
A boolean flag indicating whether to turn on global fencing shall be passed down to the txn commit sender to determine whether we should include (member.id, instance.id, generation.id) in the request.

This separation is to avoid if-else loop complexity warning from checkstyle.

When working with the multi-version API, I realized that by making the data initialization internal could save a lot of caller's effort.

The test coverage for KafkaProducerTest is weak in general. We just did the bare minimum here to route the request through a full init->begin->commit->end workflow and make sure it is working properly.

This is just leveraging the same security check here, no harm to do for both API calls.

The 3 tests here are primarily evaluating that when we are on groupMetadata mode, we could correctly detect FENCED_INSTANCE_ID, UNKNOWN_MEMBER_ID and ILLEGAL_GENERATION exceptions.

A full test to set all 3 group fields

Use new API for compatibility test.

Same here for compatibility.

I'm wondering if we should use CommitFailedException. In the consumer, we do not expose illegal generation and unknown member id errors directly to the user.

That's a good suggestion, I could wrap unknown member id and illegal generation by a commit failed exception.

nit: I think we can do away with enableGroupFencing and derive its value from ConsumerGroupMetadata. In spite of my comment on the previous PR, it may be simpler to just let the defaults be consistent with the expected default values and just have one path below.

How do we distinguish with the case where only groupId is passed, v.s. the whole groupMetadata is passed but other fields are UNKNOWN_XXX? Do we guarantee that groupMetadata#generationId should never be UNKNOWN_GENERATION_ID (from the broker-side logic we would not check if the generationId < 0)? If yes then we can use that as the boolean flag and get rid of enableGroupFencing.

I think we should validate the object when it is received in sendOffsets.

Why this change? Should sendOffsetsToTransaction not be able to handle null gracefully? Seems it would be a regression if we change the behavior and start to fail on null?

It is actually not, I will try to specify the object type so we don't need to use 0 length string

How do we distinguish with the case where only groupId is passed, v.s. the whole groupMetadata is passed but other fields are UNKNOWN_XXX? Do we guarantee that groupMetadata#generationId should never be UNKNOWN_GENERATION_ID (from the broker-side logic we would not check if the generationId < 0)? If yes then we can use that as the boolean flag and get rid of enableGroupFencing.

Test changes in this class are only for new API coverage.

After some thoughts, I feel hesitated to classify the FencedInstanceId as a sub type of CommitFailed, for producer exception handling we should abort the current transaction and let consumer rejoin the group as needed. For instanceId fenced, it is more fatal as an indicator of a malicious client that should fail the entire client. Like @hachikuji proposed, it makes sense for us to specify new EOS example code once the changes are merged so that we could make sure the API is user friendly: https://issues.apache.org/jira/browse/KAFKA-9447

It's not really valid to commit offsets with a null groupId. Why don't we use requireNonNull?

We could do that, it's just a legacy logic for allowing null groupId

It never actually made sense since the producer itself doesn't support it.

I think there's probably a good case to raise this one directly as an abortable error instead of getting wrapped in CommitFailedException. Although it is not fatal for the producer, the user shouldn't ignore it.

User will have to handle this exception properly by either:

1. aborting the transaction as recommended
2. fail the entire application to be more strict
3. other cases as they see fit

So no matter how to handle it, the error shall not be ignored. If we are throwing 3 different types of exceptions, user would as well need to catch 3 different types of exceptions IMHO

My expectation for illegal generation and unknown member id is that it can be more or less ignored. The user should abort the transaction, but then continue after rejoining the group. The instance fenced error means a new instance of the application has been started somewhere and the application should be stopped.

By the way, this is why I suggested writing some example code which shows what we consider to be proper handling. This will give us a better idea if the handling is reasonable, awkward, or incomplete.

That's true, we also had a jira to track it here: https://issues.apache.org/jira/browse/KAFKA-9447

These test cases seem to be identical code other than the error. Can we factor out a helper?

Actually there are a couple of differences inside the test, such as error type, consumer metadata creation, and request matcher. Probably we could just leave as it is since new readers would just fix one by reading through the whole block

nit: we may as well move this check into ConsumerGroupMetadata since we have some other null checks there.

The check is a little bit over-specific for ConsumerGroupMetadata itself, which is why I put advanced check in here so that people could construct group metadata in error format as they want.

Can you elaborate why this is different from e.g. the memberId?

I think these should all be IllegalArgumentException. The producer is not in an illegal state.

Wait, how did we end up back here? I thought we agreed this should not be fatal for the producer? I think it should have a separate branch above, similar to the handling of GROUP_AUTHORIZATION_FAILED.