Skip to content

Conversation

arvi18
Copy link

@arvi18 arvi18 commented Apr 21, 2025

WIP

Copy link

coderabbitai bot commented Apr 21, 2025

Warning

Rate limit exceeded

@arvi18 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 41 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between ed79212 and 8c11aee.

📒 Files selected for processing (7)
  • clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java (5 hunks)
  • clients/src/main/resources/common/message/OffsetFetchRequest.json (2 hunks)
  • clients/src/main/resources/common/message/OffsetFetchResponse.json (3 hunks)
  • core/src/main/scala/kafka/server/KafkaApis.scala (4 hunks)
  • core/src/test/scala/unit/kafka/server/KafkaApisTest.scala (17 hunks)
  • group-coordinator/src/main/java/org/apache/kafka/coordinator/group/OffsetMetadataManager.java (3 hunks)
  • group-coordinator/src/test/java/org/apache/kafka/coordinator/group/OffsetMetadataManagerTest.java (1 hunks)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@visz11
Copy link
Collaborator

visz11 commented May 27, 2025

/codehelper

1 similar comment
@visz11
Copy link
Collaborator

visz11 commented May 27, 2025

/codehelper

@ParameterizedTest
@ApiKeyVersionsSource(apiKey = ApiKeys.OFFSET_FETCH)
def testHandleOffsetFetchWithMultipleGroups(version: Short): Unit = {
val foo = "foo"
Copy link

Choose a reason for hiding this comment

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

Unused variable in test method: The variable bar is declared but not used in this test method. Consider removing it or using it in the test to avoid confusion.


private def fetchOffsetsForGroup(
requestContext: RequestContext,
offsetFetchRequest: OffsetFetchRequestData.OffsetFetchRequestGroup,
Copy link

Choose a reason for hiding this comment

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

Topic ID Support Version Handling: Please verify that the implementation correctly handles both topic IDs and topic names across different API versions, especially around version 10 which introduces topic ID support. The conditional logic paths based on version checks need careful verification to ensure backward compatibility is maintained.

.setGroupId(offsetFetchRequest.groupId)
.setErrorCode(Errors.forException(exception).code)
} else if (offsetFetchResponse.errorCode() != Errors.NONE.code) {
offsetFetchResponse
Copy link

Choose a reason for hiding this comment

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

Authorization Logic with Topic IDs: This section modifies authorization logic to handle topic IDs. Please confirm that authorization checks work properly when using topic IDs instead of topic names, as this is security-critical functionality.

.setErrorCode(Errors.INVALID_GROUP_ID.code)

val group4Response = new OffsetFetchResponseData.OffsetFetchResponseGroup()
.setGroupId("group-4")
Copy link

Choose a reason for hiding this comment

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

Test Coverage Suggestion: Consider adding specific tests for version compatibility edge cases. The PR adds version-specific behavior for TopicId support (version >= 10), but test coverage for version boundary transitions could be strengthened.

@arvi18
Copy link
Author

arvi18 commented Aug 11, 2025

/refacto-test

Copy link

refacto-test bot commented Aug 11, 2025

Refacto is reviewing this PR. Please wait for the review comments to be posted.

Copy link

refacto-test bot commented Aug 11, 2025

Code Review: Kafka OffsetFetch API Topic ID Support

👍 Well Done
Backward Compatibility

Maintains backward compatibility with version checks for older clients while adding topic ID support.

Error Handling

Properly handles unknown topic IDs with appropriate error codes.

📌 Files Processed
  • core/src/test/scala/unit/kafka/server/KafkaApisTest.scala
  • core/src/main/scala/kafka/server/KafkaApis.scala
  • clients/src/main/resources/common/message/OffsetFetchRequest.json
  • clients/src/main/resources/common/message/OffsetFetchResponse.json
  • group-coordinator/src/test/java/org/apache/kafka/coordinator/group/OffsetMetadataManagerTest.java
  • clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java
  • group-coordinator/src/main/java/org/apache/kafka/coordinator/group/OffsetMetadataManager.java
📝 Additional Comments
core/src/main/scala/kafka/server/KafkaApis.scala (3)
Defensive Topic Validation

Missing null check on topic.name could cause NullPointerException. Malformed requests with null topic names could crash the server.

    offsetFetchRequest.topics.forEach { topic =>
      if (useTopicIds && (topic.name == null || topic.name.isEmpty)) {
        errorTopics += buildErrorResponse(topic, Errors.UNKNOWN_TOPIC_ID)
      } else if (!authorizedTopicNames.contains(topic.name)) {
        errorTopics += buildErrorResponse(topic, Errors.TOPIC_AUTHORIZATION_FAILED)
      } else {
        authorizedTopics += topic
      }
    }

Standards:

  • CWE-476
  • OWASP-A06
Redundant Topic Lookups

Topic name lookups are performed for every topic in the request, but results aren't cached. With many topics, this causes repeated metadata cache lookups, increasing latency.

// Create a mapping once to avoid repeated lookups
val topicIdToName = if (useTopicIds) {
  val idNameMap = new java.util.HashMap[Uuid, String]()
  offsetFetchRequest.topics.forEach { topic =>
    if (topic.topicId != Uuid.ZERO_UUID && topic.name.isEmpty) {
      metadataCache.getTopicName(topic.topicId).ifPresent { name =>
        topic.setName(name)
        idNameMap.put(topic.topicId, name)
      }
    }
  }
  idNameMap
} else java.util.Collections.emptyMap[Uuid, String]()

Standards:

  • ISO-IEC-25010-Performance-Resource-Utilization
  • Netflix-Hot-Path-Optimization
  • Algorithm-Opt-Caching
Inefficient Collection Conversion

Converting between Java and Scala collections creates unnecessary object allocations. With large topic sets, this increases GC pressure and memory usage.

val topics = new java.util.ArrayList[OffsetFetchResponseData.OffsetFetchResponseTopics](offsetFetchResponse.topics.size())
offsetFetchResponse.topics.forEach { topic =>
  if (authorizedNames.contains(topic.name)) {
    if (useTopicIds) {
      // If the topic is not provided by the group coordinator, we set it
      // using the metadata cache.
      if (topic.topicId == Uuid.ZERO_UUID) {
        topic.setTopicId(metadataCache.getTopicId(topic.name))
      }
      // If we don't have the topic id at all, we skip the topic because
      // we can not serialize it without it.
      if (topic.topicId != Uuid.ZERO_UUID) {
        topics.add(topic)
      }
    } else {
      topics.add(topic)
    }
  }
}
offsetFetchResponse.setTopics(topics)

Standards:

  • ISO-IEC-25010-Performance-Resource-Utilization
  • Netflix-Memory-Optimization
  • Algorithm-Opt-Collection-Efficiency
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/OffsetMetadataManager.java (1)
Missing Version Check

No version check before setting topicId in fetchAllOffsets. While this works because higher layers handle version compatibility, defensive programming would include version awareness here.

                    new OffsetFetchResponseData.OffsetFetchResponseTopics()
                        // The topicId is set to ZERO_UUID initially.
                        // Version compatibility is handled at the API layer that will
                        // either use or ignore this field based on the client version.
                        .setTopicId(Uuid.ZERO_UUID)
                        .setName(topic)

Standards:

  • ISO-IEC-25010-Reliability-Maturity
  • ISO-IEC-25010-Functional-Correctness-Appropriateness
  • DbC-Documentation
  • SRE-Code-Clarity

Comment on lines +1084 to +1088
offsetFetchRequest.topics.forEach { topic =>
if (topic.topicId != Uuid.ZERO_UUID) {
metadataCache.getTopicName(topic.topicId).ifPresent(name => topic.setName(name))
}
}
Copy link

Choose a reason for hiding this comment

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

Null Check Missing

Missing null check on offsetFetchRequest.topics could cause NullPointerException. Attackers could send malformed requests with null topics to trigger server errors.

Suggested change
offsetFetchRequest.topics.forEach { topic =>
if (topic.topicId != Uuid.ZERO_UUID) {
metadataCache.getTopicName(topic.topicId).ifPresent(name => topic.setName(name))
}
}
if (useTopicIds) {
if (offsetFetchRequest.topics != null) {
offsetFetchRequest.topics.forEach { topic =>
if (topic.topicId != Uuid.ZERO_UUID) {
metadataCache.getTopicName(topic.topicId).ifPresent(name => topic.setName(name))
}
}
}
}
Standards
  • CWE-476
  • OWASP-A06

Comment on lines +1045 to 1050
val authorizedNames = authHelper.filterByAuthorized(
requestContext,
DESCRIBE,
TOPIC,
offsetFetchResponse.topics.asScala
)(_.name)
Copy link

Choose a reason for hiding this comment

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

Null Check Missing

No null check on offsetFetchResponse.topics before conversion to Scala collection. Could cause NullPointerException if topics is null, enabling denial of service attacks.

Suggested change
val authorizedNames = authHelper.filterByAuthorized(
requestContext,
DESCRIBE,
TOPIC,
offsetFetchResponse.topics.asScala
)(_.name)
val authorizedNames = if (offsetFetchResponse.topics == null) {
Set.empty[String]
} else {
authHelper.filterByAuthorized(
requestContext,
DESCRIBE,
TOPIC,
offsetFetchResponse.topics.asScala
)(_.name)
}
Standards
  • CWE-476
  • OWASP-A06

Comment on lines +1083 to +1089
if (useTopicIds) {
offsetFetchRequest.topics.forEach { topic =>
if (topic.topicId != Uuid.ZERO_UUID) {
metadataCache.getTopicName(topic.topicId).ifPresent(name => topic.setName(name))
}
}
}
Copy link

Choose a reason for hiding this comment

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

Missing TopicId Validation

Missing validation when topic ID is present but name lookup fails. This creates inconsistency where topicId exists but name remains empty, causing potential reliability issues.

Suggested change
if (useTopicIds) {
offsetFetchRequest.topics.forEach { topic =>
if (topic.topicId != Uuid.ZERO_UUID) {
metadataCache.getTopicName(topic.topicId).ifPresent(name => topic.setName(name))
}
}
}
if (useTopicIds) {
offsetFetchRequest.topics.forEach { topic =>
if (topic.topicId != Uuid.ZERO_UUID) {
val topicNameOpt = metadataCache.getTopicName(topic.topicId)
if (topicNameOpt.isPresent) {
topic.setName(topicNameOpt.get)
} else {
// If we can't resolve the topic ID, mark it with empty name
// It will be handled later with UNKNOWN_TOPIC_ID error
topic.setName("")
}
}
}
}
Standards
  • ISO-IEC-25010-Reliability-Fault-Tolerance
  • ISO-IEC-25010-Functional-Correctness-Appropriateness
  • DbC-Precondition-Validation
  • SRE-Error-Handling

Comment on lines +1102 to +1116
def buildErrorResponse(
topic: OffsetFetchRequestData.OffsetFetchRequestTopics,
error: Errors
): OffsetFetchResponseData.OffsetFetchResponseTopics = {
val topicResponse = new OffsetFetchResponseData.OffsetFetchResponseTopics()
.setTopicId(topic.topicId)
.setName(topic.name)
topic.partitionIndexes.forEach { partitionIndex =>
topicResponse.partitions.add(new OffsetFetchResponseData.OffsetFetchResponsePartitions()
.setPartitionIndex(partitionIndex)
.setCommittedOffset(-1)
.setErrorCode(error.code))
}
topicResponse
}
Copy link

Choose a reason for hiding this comment

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

Incomplete Error Handling

Error response builder doesn't set committed leader epoch. This inconsistency could lead to client-side errors when processing responses with errors, as other code paths set this field.

Suggested change
def buildErrorResponse(
topic: OffsetFetchRequestData.OffsetFetchRequestTopics,
error: Errors
): OffsetFetchResponseData.OffsetFetchResponseTopics = {
val topicResponse = new OffsetFetchResponseData.OffsetFetchResponseTopics()
.setTopicId(topic.topicId)
.setName(topic.name)
topic.partitionIndexes.forEach { partitionIndex =>
topicResponse.partitions.add(new OffsetFetchResponseData.OffsetFetchResponsePartitions()
.setPartitionIndex(partitionIndex)
.setCommittedOffset(-1)
.setErrorCode(error.code))
}
topicResponse
}
def buildErrorResponse(
topic: OffsetFetchRequestData.OffsetFetchRequestTopics,
error: Errors
): OffsetFetchResponseData.OffsetFetchResponseTopics = {
val topicResponse = new OffsetFetchResponseData.OffsetFetchResponseTopics()
.setTopicId(topic.topicId)
.setName(topic.name)
topic.partitionIndexes.forEach { partitionIndex =>
topicResponse.partitions.add(new OffsetFetchResponseData.OffsetFetchResponsePartitions()
.setPartitionIndex(partitionIndex)
.setCommittedOffset(-1)
.setCommittedLeaderEpoch(-1)
.setErrorCode(error.code))
}
topicResponse
}
Standards
  • ISO-IEC-25010-Reliability-Maturity
  • ISO-IEC-25010-Functional-Correctness-Completeness
  • DbC-Postcondition-Consistency
  • SRE-Error-Handling

Comment on lines +743 to +745
new OffsetFetchResponseData.OffsetFetchResponseTopics()
.setTopicId(topic.topicId())
.setName(topic.name());
Copy link

Choose a reason for hiding this comment

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

Inconsistent Name Handling

Topic name is set inconsistently in the response. When using topic IDs, the response must include both ID and name for backward compatibility and proper client-side handling.

Suggested change
new OffsetFetchResponseData.OffsetFetchResponseTopics()
.setTopicId(topic.topicId())
.setName(topic.name());
new OffsetFetchResponseData.OffsetFetchResponseTopics()
.setTopicId(topic.topicId())
.setName(topic.name())
Standards
  • ISO-IEC-25010-Reliability-Maturity
  • ISO-IEC-25010-Functional-Correctness-Appropriateness
  • DbC-Contract-Consistency
  • SRE-Backward-Compatibility

Comment on lines +1045 to 1050
val authorizedNames = authHelper.filterByAuthorized(
requestContext,
DESCRIBE,
TOPIC,
offsetFetchResponse.topics.asScala
)(_.name)
Copy link

Choose a reason for hiding this comment

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

Unnecessary String Comparisons

Filtering by topic names causes unnecessary string comparisons when topic IDs are available. With many topics, this creates O(n) lookups where O(1) is possible using topic IDs directly.

Suggested change
val authorizedNames = authHelper.filterByAuthorized(
requestContext,
DESCRIBE,
TOPIC,
offsetFetchResponse.topics.asScala
)(_.name)
val authorizedIdentifiers = if (useTopicIds) {
authHelper.filterByAuthorized(requestContext, DESCRIBE, TOPIC, offsetFetchResponse.topics.asScala)(topic => metadataCache.getTopicName(topic.topicId).orElse(topic.name))
} else {
authHelper.filterByAuthorized(requestContext, DESCRIBE, TOPIC, offsetFetchResponse.topics.asScala)(_.name)
}
Standards
  • ISO-IEC-25010-Performance-Time-Behaviour
  • Algorithm-Opt-Hash-Map
  • Netflix-Hot-Path-Optimization

@arvi18
Copy link
Author

arvi18 commented Aug 11, 2025

Reopening this in a new PR

@arvi18 arvi18 closed this Aug 11, 2025
@arvi18 arvi18 reopened this Aug 11, 2025
@visz11
Copy link
Collaborator

visz11 commented Aug 22, 2025

/refacto-test

Copy link

refacto-test bot commented Aug 22, 2025

Refacto is reviewing this PR. Please wait for the review comments to be posted.

@visz11
Copy link
Collaborator

visz11 commented Aug 22, 2025

/refacto-test

@visz11
Copy link
Collaborator

visz11 commented Sep 11, 2025

/refacto-test-arvi

Copy link

refacto-visz bot commented Sep 11, 2025

Refacto is reviewing this PR. Please wait for the review comments to be posted.

Copy link

refacto-visz bot commented Sep 11, 2025

Code Review: OffsetFetch API Topic ID Support

👍 Well Done
Comprehensive TopicId Support

Consistent implementation across request/response objects enhances system reliability.

Error Handling

Added UNKNOWN_TOPIC_ID error handling prevents system failures.

Backward Compatibility

Version checks maintain compatibility while extending functionality.

📌 Files Processed
  • core/src/test/scala/unit/kafka/server/KafkaApisTest.scala
  • core/src/main/scala/kafka/server/KafkaApis.scala
  • clients/src/main/resources/common/message/OffsetFetchRequest.json
  • clients/src/main/resources/common/message/OffsetFetchResponse.json
  • group-coordinator/src/test/java/org/apache/kafka/coordinator/group/OffsetMetadataManagerTest.java
  • clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java
  • group-coordinator/src/main/java/org/apache/kafka/coordinator/group/OffsetMetadataManager.java
📝 Additional Comments
core/src/main/scala/kafka/server/KafkaApis.scala (5)
Inconsistent Response Serialization

When useTopicIds=true, topics with unresolvable IDs are silently dropped from response without error indication. This differs from request handling which returns UNKNOWN_TOPIC_ID error, creating inconsistent error reporting behavior across request/response paths.

Standards:

  • Logic-Verification-Consistency
  • Business-Rule-Completeness
  • Algorithm-Correctness-ErrorHandling
Error Handling Duplication

Error handling logic is duplicated between fetchOffsets and fetchAllOffsets methods. Consider extracting this helper to a shared location for reuse in other API handlers that process topic responses.

Standards:

  • Clean-Code-DRY
  • Design-Pattern-Template
Conditional Topic Filtering

Nested conditionals create branching complexity and potential cache misses. Consider refactoring to use a more functional approach with filter operations and pattern matching to improve code clarity and reduce branch prediction failures in the CPU pipeline.

Standards:

  • ISO-IEC-25010-Performance-Time-Behaviour
  • Algorithm-Opt-Branch-Reduction
Potential Empty Topics

No null check on offsetFetchRequest.topics before processing. If topics collection is null, this could cause NullPointerException during request processing, affecting client operations.

Standards:

  • ISO-IEC-25010-Reliability-Fault-Tolerance
  • ISO-IEC-25010-Functional-Correctness-Appropriateness
Version Check Abstraction

Version check logic is duplicated across multiple methods. Consider extracting version-specific behavior into a strategy pattern to centralize version-dependent logic and improve maintainability when adding new versions.

Standards:

  • Clean-Code-DRY
  • Design-Pattern-Strategy
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala (2)
Redundant Name Check

Test uses conditional logic to set name to empty string when version >= 10, but this pattern is inconsistently applied across test cases. Some tests set both name and ID regardless of version, potentially masking version-specific serialization bugs.

Standards:

  • Logic-Verification-TestConsistency
  • Algorithm-Correctness-TestCoverage
Test Helper Methods

Test setup code is duplicated across multiple test methods. Consider extracting common setup logic into helper methods to improve test maintainability and readability when adding new test cases.

Standards:

  • Clean-Code-DRY
  • Clean-Code-Test-Organization

Comment on lines +1082 to +1088

if (useTopicIds) {
offsetFetchRequest.topics.forEach { topic =>
if (topic.topicId != Uuid.ZERO_UUID) {
metadataCache.getTopicName(topic.topicId).ifPresent(name => topic.setName(name))
}
}
Copy link

Choose a reason for hiding this comment

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

Topic ID Inconsistency

Topic name resolution only happens when topicId is non-zero, but later code at line 1119 checks for empty name regardless of ID value. This creates inconsistent validation where topics with ZERO_UUID but empty names trigger UNKNOWN_TOPIC_ID error incorrectly.

Standards
  • Logic-Verification-Consistency
  • Business-Rule-Validation
  • Algorithm-Correctness-Conditional

Comment on lines +1082 to +1089

if (useTopicIds) {
offsetFetchRequest.topics.forEach { topic =>
if (topic.topicId != Uuid.ZERO_UUID) {
metadataCache.getTopicName(topic.topicId).ifPresent(name => topic.setName(name))
}
}
}
Copy link

Choose a reason for hiding this comment

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

Redundant Topic Name

Setting topic names when using topic IDs creates unnecessary overhead. When using topic IDs (version >= 10), the name isn't required for lookups. This redundant operation adds CPU and memory overhead by populating fields that won't be used in the authorization path.

Standards
  • ISO-IEC-25010-Performance-Resource-Utilization
  • Algorithm-Opt-Redundant-Operation

Comment on lines +1118 to +1122
offsetFetchRequest.topics.forEach { topic =>
if (useTopicIds && topic.name.isEmpty) {
errorTopics += buildErrorResponse(topic, Errors.UNKNOWN_TOPIC_ID)
} else if (!authorizedTopicNames.contains(topic.name)) {
errorTopics += buildErrorResponse(topic, Errors.TOPIC_AUTHORIZATION_FAILED)
Copy link

Choose a reason for hiding this comment

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

Incomplete Error Handling

Logic fails to handle case where topic.name is non-empty but invalid/non-existent. Authorization check will fail but with incorrect error code (TOPIC_AUTHORIZATION_FAILED instead of UNKNOWN_TOPIC_ID) when topic ID is provided but resolves to non-existent name.

Standards
  • Business-Rule-Validation
  • Logic-Verification-Completeness
  • Algorithm-Correctness-ErrorHandling

Comment on lines +8238 to +8254
val expectedGroup1Response = new OffsetFetchResponseData.OffsetFetchResponseGroup()
.setGroupId("group-1")
.setTopics(List(
new OffsetFetchResponseData.OffsetFetchResponseTopics()
.setTopicId(if (version >= 10) fooId else Uuid.ZERO_UUID)
.setName(if (version < 10) foo else "")
.setPartitions(List(
new OffsetFetchResponseData.OffsetFetchResponsePartitions()
.setPartitionIndex(0)
.setCommittedOffset(100)
.setCommittedLeaderEpoch(1),
new OffsetFetchResponseData.OffsetFetchResponsePartitions()
.setPartitionIndex(1)
.setCommittedOffset(200)
.setCommittedLeaderEpoch(2)
).asJava)
).asJava)
Copy link

Choose a reason for hiding this comment

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

Duplicated Response Mapping

Duplicated response object construction pattern appears multiple times in the test. This creates maintenance burden when response structure changes. Extract helper method to build response objects with version-specific fields.

Standards
  • Clean-Code-DRY
  • Design-Pattern-Factory

Comment on lines +8242 to +8243
.setTopicId(if (version >= 10) fooId else Uuid.ZERO_UUID)
.setName(if (version < 10) foo else "")
Copy link

Choose a reason for hiding this comment

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

Conditional Field Setting

Conditional logic for version-specific field values is repeated across multiple test cases. This creates maintenance overhead when version logic changes. Extract version-specific field setting into helper methods.

Standards
  • Clean-Code-DRY
  • Design-Pattern-Strategy

Comment on lines +1099 to +1126
val authorizedTopics = new mutable.ArrayBuffer[OffsetFetchRequestData.OffsetFetchRequestTopics]
val errorTopics = new mutable.ArrayBuffer[OffsetFetchResponseData.OffsetFetchResponseTopics]

def buildErrorResponse(
topic: OffsetFetchRequestData.OffsetFetchRequestTopics,
error: Errors
): OffsetFetchResponseData.OffsetFetchResponseTopics = {
val topicResponse = new OffsetFetchResponseData.OffsetFetchResponseTopics()
.setTopicId(topic.topicId)
.setName(topic.name)
topic.partitionIndexes.forEach { partitionIndex =>
topicResponse.partitions.add(new OffsetFetchResponseData.OffsetFetchResponsePartitions()
.setPartitionIndex(partitionIndex)
.setCommittedOffset(-1)
.setErrorCode(error.code))
}
topicResponse
}

offsetFetchRequest.topics.forEach { topic =>
if (useTopicIds && topic.name.isEmpty) {
errorTopics += buildErrorResponse(topic, Errors.UNKNOWN_TOPIC_ID)
} else if (!authorizedTopicNames.contains(topic.name)) {
errorTopics += buildErrorResponse(topic, Errors.TOPIC_AUTHORIZATION_FAILED)
} else {
authorizedTopics += topic
}
}
Copy link

Choose a reason for hiding this comment

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

Inefficient Collection Conversion

Multiple collection conversions and intermediate buffers create unnecessary overhead. The code creates temporary buffers and performs multiple iterations over the topic collection. Consider using partition/filter operations to categorize topics in a single pass, reducing memory allocations and CPU cycles.

Standards
  • ISO-IEC-25010-Performance-Resource-Utilization
  • Algorithm-Opt-Collection-Processing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants