Skip to content

refactor(go): Introduce Command interface to improve Go client request handling#2737

Merged
mmodzelewski merged 16 commits intoapache:masterfrom
chengxilo:refactor-use-do-instead
Mar 2, 2026
Merged

refactor(go): Introduce Command interface to improve Go client request handling#2737
mmodzelewski merged 16 commits intoapache:masterfrom
chengxilo:refactor-use-do-instead

Conversation

@chengxilo
Copy link
Contributor

@chengxilo chengxilo commented Feb 14, 2026

Which issue does this PR close?

Closes # N/A


Rationale

Currently, the Go client performs web requests by calling:

func (c *IggyTcpClient) sendAndFetchResponse(message []byte, command iggcon.CommandCode) ([]byte, error)

While this works, it tightly couples request serialization and command code handling at the call site, making the API less expressive and harder to extend.


What changed?

Previously, each web request manually passed serialized bytes together with a CommandCode into sendAndFetchResponse, requiring callers to manage both serialization and command metadata.

Now, we use:

func (c *IggyTcpClient) do(cmd iggcon.Command) ([]byte, error)

Request types implement a new Command interface that:

  • Supports binary serialization (Implement MarshalBinary method)
  • Provides a Code() method to return the associated command code

This moves command metadata and serialization logic into the request type itself, improving encapsulation, readability, and extensibility.

Command interface and do function are already implemented in previous commit


Local Execution

  • Passed locally
  • Pre-commit hooks ran

AI Usage

  • Tool: GitHub Copilot and ChatGPT
  • Scope: Copilot is for autocomplete assistance only. I use ChatGPT to help with writing(Yeah this pr)
  • Verification: All changes were compiled and tested locally
  • Yes, I can explain every line of the implementation if needed.

@chengxilo chengxilo changed the title refactor(go): Introduce Command abstraction to improve Go client request handling refactor(go): Introduce Command interface to improve Go client request handling Feb 14, 2026
@chengxilo chengxilo force-pushed the refactor-use-do-instead branch 2 times, most recently from 9229f45 to f9fbb90 Compare February 16, 2026 16:57
@chengxilo chengxilo marked this pull request as ready for review February 16, 2026 17:23
@chengxilo
Copy link
Contributor Author

@ex172000 regarding the request for tests and the common struct:

I actually hesitated on this because the current foreign/go/contract directory is already quite cluttered, with types for various purposes mixed together.

To avoid suffering in the cluttered types, I believe we should reorganize the directory structure in foreign/go to better categorize these types. However, if I include that reorganization and the new tests in this PR, the "lines changed" will explode, making this a >3000 lines of change(or even more, I don't know).

My proposal:

  1. I will fix the naming consistency for ConsumerGroupId/GroupId now. And probably introduce the common struct.
  2. We merge this PR.
  3. After that, I will submit some follow-up PR focused solely on reorganizing the directory and adding the unit tests you suggested.

I think it would make the review process easier. What do you think?

@ex172000
Copy link
Contributor

@chengxilo sgtm! Yeah 3000+ LoC is definitely too large to review! It's just the byte operations are easy to go wrong in the future edits thus testing could be helpful!

@chengxilo
Copy link
Contributor Author

Hey @ex172000, I added that common struct TopicPath for the group commands. I didn't embed it into ConsumerGroupInfo on purpose as we might split these into different packages later. I’d rather keep the request types and the data models separate so they don't get all tangled up.

@chengxilo chengxilo requested a review from ex172000 February 18, 2026 18:17
@ex172000
Copy link
Contributor

Hey @ex172000, I added that common struct TopicPath for the group commands. I didn't embed it into ConsumerGroupInfo on purpose as we might split these into different packages later. I’d rather keep the request types and the data models separate so they don't get all tangled up.

Thanks for the update! Just wondering, some of the fields are still having JSON annotation, but new ones don't. Do we need it or we can drop it completely?

@chengxilo
Copy link
Contributor Author

chengxilo commented Feb 19, 2026

Hey @ex172000, I added that common struct TopicPath for the group commands. I didn't embed it into ConsumerGroupInfo on purpose as we might split these into different packages later. I’d rather keep the request types and the data models separate so they don't get all tangled up.

Thanks for the update! Just wondering, some of the fields are still having JSON annotation, but new ones don't. Do we need it or we can drop it completely?

@ex172000

Thank you for mentioning that.

The JSON annotations seem to be autogenerated code from previous maintainer. They actually do nothing since HTTP is not implemented yet. I don't think we will implement the HTTP in go within a short term of time.

Since currently they are useless and may not align with the JSON we actually need in HTTP SDK, maybe I can remove them all in another PR.🤔

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs.

If you need a review, please ensure CI is green and the PR is rebased on the latest master. Don't hesitate to ping the maintainers - either @core on Discord or by mentioning them directly here on the PR.

Thank you for your contribution!

@github-actions github-actions bot added the stale Inactive issue or pull request label Feb 27, 2026
Copy link
Contributor

@ex172000 ex172000 left a comment

Choose a reason for hiding this comment

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

I don't have write permission then I guess this review won't count.
Hope @hubcio can bring someone to review Golang changes

@chengxilo
Copy link
Contributor Author

I don't have write permission then I guess this review won't count. Hope @hubcio can bring someone to review Golang changes

Thanks. It's ok, I have to solve the conflicts first anyway.🤣

@chengxilo chengxilo force-pushed the refactor-use-do-instead branch from bb6d83a to 259281b Compare February 27, 2026 04:21
@codecov
Copy link

codecov bot commented Feb 27, 2026

Codecov Report

❌ Patch coverage is 10.40724% with 594 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.41%. Comparing base (ab8834d) to head (daf52e2).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
foreign/go/contracts/offsets.go 0.00% 85 Missing ⚠️
foreign/go/contracts/users.go 0.00% 66 Missing ⚠️
foreign/go/contracts/update_user.go 0.00% 41 Missing ⚠️
...ign/go/client/tcp/tcp_consumer_group_management.go 0.00% 38 Missing ⚠️
foreign/go/contracts/consumer_groups.go 0.00% 36 Missing ⚠️
foreign/go/contracts/create_user.go 0.00% 36 Missing ⚠️
foreign/go/contracts/partitions.go 15.00% 34 Missing ⚠️
foreign/go/contracts/update_user_permissions.go 0.00% 25 Missing ⚠️
foreign/go/contracts/access_tokens.go 0.00% 21 Missing ⚠️
foreign/go/contracts/change_password.go 0.00% 19 Missing ⚠️
... and 32 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2737      +/-   ##
============================================
- Coverage     67.70%   67.41%   -0.29%     
+ Complexity      739      708      -31     
============================================
  Files          1031     1044      +13     
  Lines         83912    83336     -576     
  Branches      60705    60004     -701     
============================================
- Hits          56810    56183     -627     
- Misses        24753    24804      +51     
  Partials       2349     2349              
Flag Coverage Δ
go 6.33% <10.40%> (-0.51%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
foreign/go/client/tcp/tcp_clients_management.go 0.00% <0.00%> (ø)
foreign/go/client/tcp/tcp_partition_management.go 0.00% <0.00%> (ø)
foreign/go/client/tcp/tcp_utilities.go 0.00% <0.00%> (ø)
foreign/go/contracts/create_stream.go 75.00% <60.00%> (ø)
...reign/go/client/tcp/tcp_access_token_management.go 0.00% <0.00%> (ø)
foreign/go/client/tcp/tcp_offset_management.go 0.00% <0.00%> (ø)
...nary_serialization/binary_response_deserializer.go 0.00% <0.00%> (ø)
foreign/go/client/tcp/tcp_session_management.go 0.00% <0.00%> (ø)
foreign/go/contracts/delete_stream.go 0.00% <0.00%> (ø)
foreign/go/contracts/delete_topic.go 0.00% <0.00%> (ø)
... and 32 more

... and 13 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hubcio
Copy link
Contributor

hubcio commented Feb 27, 2026

@ex172000 @chengxilo what's the state of this PR? do you believe that it's acceptable quality for merge?

@chengxilo
Copy link
Contributor Author

chengxilo commented Feb 27, 2026

@ex172000 @chengxilo what's the state of this PR? do you believe that it's acceptable quality for merge?

I believe it's acceptable for now.
To keep the review manageable, I'll be submitting separate PR shortly after this to reorganize the directory and add the missing tests.
(The Codecov Report is terrifying tho)

@ex172000
Copy link
Contributor

@ex172000 @chengxilo what's the state of this PR? do you believe that it's acceptable quality for merge?

I think the overall structure for this PR is good and @chengxilo has addressed many of the feedback. We anyway need more follow up PRs to further the refactoring/testing.

@github-actions github-actions bot removed the stale Inactive issue or pull request label Feb 28, 2026
@mmodzelewski mmodzelewski merged commit 2eefbc3 into apache:master Mar 2, 2026
46 checks passed
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.

5 participants