You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iggy.apache.org by "chengxilo (via GitHub)" <gi...@apache.org> on 2026/02/14 04:20:32 UTC
[PR] refactor(go): Introduce Command abstraction to improve Go client request handling [iggy]
chengxilo opened a new pull request, #2737:
URL: https://github.com/apache/iggy/pull/2737
## Which issue does this PR close?
Closes # N/A
---
## Rationale
Currently, the Go client performs web requests by calling:
```go
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?
This PR depends on https://github.com/apache/iggy/pull/2580.
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:
```go
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.
---
## Local Execution
- Passed locally
- Pre-commit hooks ran
Not ready for review yet.
---
## 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.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(go): Introduce Command interface to improve Go client request handling [iggy]
Posted by "ex172000 (via GitHub)" <gi...@apache.org>.
ex172000 commented on code in PR #2737:
URL: https://github.com/apache/iggy/pull/2737#discussion_r2819254897
##########
foreign/go/contracts/consumer_groups.go:
##########
@@ -35,30 +35,103 @@ type ConsumerGroupMember struct {
Partitions []uint32
}
-type CreateConsumerGroupRequest struct {
+type CreateConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
Name string `json:"name"`
}
-type DeleteConsumerGroupRequest struct {
+func (c *CreateConsumerGroup) Code() CommandCode {
+ return CreateGroupCode
+}
+
+func (c *CreateConsumerGroup) MarshalBinary() ([]byte, error) {
+ streamIdBytes, err := c.StreamId.MarshalBinary()
+ if err != nil {
+ return nil, err
+ }
+ topicIdBytes, err := c.TopicId.MarshalBinary()
+ if err != nil {
+ return nil, err
+ }
+ offset := len(streamIdBytes) + len(topicIdBytes)
+ bytes := make([]byte, offset+1+len(c.Name))
+ copy(bytes[0:len(streamIdBytes)], streamIdBytes)
+ copy(bytes[len(streamIdBytes):offset], topicIdBytes)
+ bytes[offset] = byte(len(c.Name))
+ copy(bytes[offset+1:], c.Name)
+ return bytes, nil
+}
+
+type DeleteConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
}
-type JoinConsumerGroupRequest struct {
+func (d *DeleteConsumerGroup) Code() CommandCode {
+ return DeleteGroupCode
+}
+
+func (d *DeleteConsumerGroup) MarshalBinary() ([]byte, error) {
+ return marshalIdentifiers(d.StreamId, d.TopicId, d.ConsumerGroupId)
+}
+
+type JoinConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
}
-type LeaveConsumerGroupRequest struct {
+func (j *JoinConsumerGroup) Code() CommandCode {
+ return JoinGroupCode
+}
+
+func (j *JoinConsumerGroup) MarshalBinary() ([]byte, error) {
+ return marshalIdentifiers(j.StreamId, j.TopicId, j.ConsumerGroupId)
+}
+
+type LeaveConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
}
+func (l *LeaveConsumerGroup) Code() CommandCode {
+ return LeaveGroupCode
+}
+
+func (l *LeaveConsumerGroup) MarshalBinary() ([]byte, error) {
+ return marshalIdentifiers(l.StreamId, l.TopicId, l.ConsumerGroupId)
+}
+
+type GetConsumerGroup struct {
+ StreamId Identifier
+ TopicId Identifier
+ GroupId Identifier
Review Comment:
The name should be consistent with line 83, also do we want to have the JSON annotation?
##########
foreign/go/contracts/consumer_groups.go:
##########
@@ -35,30 +35,103 @@ type ConsumerGroupMember struct {
Partitions []uint32
}
-type CreateConsumerGroupRequest struct {
+type CreateConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
Name string `json:"name"`
}
-type DeleteConsumerGroupRequest struct {
+func (c *CreateConsumerGroup) Code() CommandCode {
+ return CreateGroupCode
+}
+
+func (c *CreateConsumerGroup) MarshalBinary() ([]byte, error) {
+ streamIdBytes, err := c.StreamId.MarshalBinary()
+ if err != nil {
+ return nil, err
+ }
+ topicIdBytes, err := c.TopicId.MarshalBinary()
+ if err != nil {
+ return nil, err
+ }
+ offset := len(streamIdBytes) + len(topicIdBytes)
+ bytes := make([]byte, offset+1+len(c.Name))
+ copy(bytes[0:len(streamIdBytes)], streamIdBytes)
+ copy(bytes[len(streamIdBytes):offset], topicIdBytes)
+ bytes[offset] = byte(len(c.Name))
+ copy(bytes[offset+1:], c.Name)
+ return bytes, nil
+}
+
+type DeleteConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
}
-type JoinConsumerGroupRequest struct {
+func (d *DeleteConsumerGroup) Code() CommandCode {
+ return DeleteGroupCode
+}
+
+func (d *DeleteConsumerGroup) MarshalBinary() ([]byte, error) {
+ return marshalIdentifiers(d.StreamId, d.TopicId, d.ConsumerGroupId)
+}
+
+type JoinConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
Review Comment:
This structure seems repeating for many CG operations, can we have a common struct ?
##########
foreign/go/contracts/create_user.go:
##########
@@ -0,0 +1,77 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package iggcon
+
+import "encoding/binary"
+
+type CreateUser struct {
+ Username string `json:"username"`
+ Password string `json:"Password"`
+ Status UserStatus `json:"Status"`
+ Permissions *Permissions `json:"Permissions,omitempty"`
+}
+
+func (c *CreateUser) Code() CommandCode {
+ return CreateUserCode
+}
+
+func (c *CreateUser) MarshalBinary() ([]byte, error) {
Review Comment:
Same here, appreciated if we can have some test
##########
foreign/go/contracts/change_password.go:
##########
@@ -0,0 +1,52 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package iggcon
+
+type ChangePassword struct {
+ UserID Identifier `json:"-"`
+ CurrentPassword string `json:"CurrentPassword"`
+ NewPassword string `json:"NewPassword"`
+}
+
+func (c *ChangePassword) Code() CommandCode {
+ return ChangePasswordCode
+}
+
+func (c *ChangePassword) MarshalBinary() ([]byte, error) {
Review Comment:
Looks quite some operations here, appreciate if we can add a test
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(go): Introduce Command interface to improve Go client request handling [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on PR #2737:
URL: https://github.com/apache/iggy/pull/2737#issuecomment-3924027492
> > 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?
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 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.🤔
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(go): Introduce Command interface to improve Go client request handling [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on PR #2737:
URL: https://github.com/apache/iggy/pull/2737#issuecomment-3922395439
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.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(go): Introduce Command interface to improve Go client request handling [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #2737:
URL: https://github.com/apache/iggy/pull/2737#discussion_r2819690017
##########
foreign/go/contracts/consumer_groups.go:
##########
@@ -35,30 +35,103 @@ type ConsumerGroupMember struct {
Partitions []uint32
}
-type CreateConsumerGroupRequest struct {
+type CreateConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
Name string `json:"name"`
}
-type DeleteConsumerGroupRequest struct {
+func (c *CreateConsumerGroup) Code() CommandCode {
+ return CreateGroupCode
+}
+
+func (c *CreateConsumerGroup) MarshalBinary() ([]byte, error) {
+ streamIdBytes, err := c.StreamId.MarshalBinary()
+ if err != nil {
+ return nil, err
+ }
+ topicIdBytes, err := c.TopicId.MarshalBinary()
+ if err != nil {
+ return nil, err
+ }
+ offset := len(streamIdBytes) + len(topicIdBytes)
+ bytes := make([]byte, offset+1+len(c.Name))
+ copy(bytes[0:len(streamIdBytes)], streamIdBytes)
+ copy(bytes[len(streamIdBytes):offset], topicIdBytes)
+ bytes[offset] = byte(len(c.Name))
+ copy(bytes[offset+1:], c.Name)
+ return bytes, nil
+}
+
+type DeleteConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
}
-type JoinConsumerGroupRequest struct {
+func (d *DeleteConsumerGroup) Code() CommandCode {
+ return DeleteGroupCode
+}
+
+func (d *DeleteConsumerGroup) MarshalBinary() ([]byte, error) {
+ return marshalIdentifiers(d.StreamId, d.TopicId, d.ConsumerGroupId)
+}
+
+type JoinConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
Review Comment:
Regarding the common struct. If I add a common strust like this:
```go
type ConsumerGroupCommandBase struct {
StreamId Identifier
TopicId Identifier
GroupId Identifier
}
```
`GetConsumerGroup` looks like this
```go
type GetConsumerGroup struct {
ConsumerGroupCommandBase
}
```
And when I use it, it would be like:
```go
buffer, err := c.do(&iggcon.GetConsumerGroup{
ConsumerGroupCommandBase: iggcon.ConsumerGroupCommandBase{
StreamId: streamId,
TopicId: topicId,
GroupId: groupId,
},
})
```
To be honest, I prefer the approach without common struct personally, when use it, it looks more straightforward.
```
buffer, err := c.do(&iggcon.GetConsumerGroup{
StreamId: streamId,
TopicId: topicId,
GroupId: groupId,
})
```
Just personal preference. What do you think?
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(go): Introduce Command interface to improve Go client request handling [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #2737:
URL: https://github.com/apache/iggy/pull/2737#discussion_r2819388666
##########
foreign/go/contracts/consumer_groups.go:
##########
@@ -35,30 +35,103 @@ type ConsumerGroupMember struct {
Partitions []uint32
}
-type CreateConsumerGroupRequest struct {
+type CreateConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
Name string `json:"name"`
}
-type DeleteConsumerGroupRequest struct {
+func (c *CreateConsumerGroup) Code() CommandCode {
+ return CreateGroupCode
+}
+
+func (c *CreateConsumerGroup) MarshalBinary() ([]byte, error) {
+ streamIdBytes, err := c.StreamId.MarshalBinary()
+ if err != nil {
+ return nil, err
+ }
+ topicIdBytes, err := c.TopicId.MarshalBinary()
+ if err != nil {
+ return nil, err
+ }
+ offset := len(streamIdBytes) + len(topicIdBytes)
+ bytes := make([]byte, offset+1+len(c.Name))
+ copy(bytes[0:len(streamIdBytes)], streamIdBytes)
+ copy(bytes[len(streamIdBytes):offset], topicIdBytes)
+ bytes[offset] = byte(len(c.Name))
+ copy(bytes[offset+1:], c.Name)
+ return bytes, nil
+}
+
+type DeleteConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
}
-type JoinConsumerGroupRequest struct {
+func (d *DeleteConsumerGroup) Code() CommandCode {
+ return DeleteGroupCode
+}
+
+func (d *DeleteConsumerGroup) MarshalBinary() ([]byte, error) {
+ return marshalIdentifiers(d.StreamId, d.TopicId, d.ConsumerGroupId)
+}
+
+type JoinConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
}
-type LeaveConsumerGroupRequest struct {
+func (j *JoinConsumerGroup) Code() CommandCode {
+ return JoinGroupCode
+}
+
+func (j *JoinConsumerGroup) MarshalBinary() ([]byte, error) {
+ return marshalIdentifiers(j.StreamId, j.TopicId, j.ConsumerGroupId)
+}
+
+type LeaveConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
}
+func (l *LeaveConsumerGroup) Code() CommandCode {
+ return LeaveGroupCode
+}
+
+func (l *LeaveConsumerGroup) MarshalBinary() ([]byte, error) {
+ return marshalIdentifiers(l.StreamId, l.TopicId, l.ConsumerGroupId)
+}
+
+type GetConsumerGroup struct {
+ StreamId Identifier
+ TopicId Identifier
+ GroupId Identifier
Review Comment:
The JSON annotation was left by the previous developers and actually has no use at all. Maybe we can remove them in the future.
And I will modify the atribute name for consistency.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(go): Introduce Command interface to improve Go client request handling [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #2737:
URL: https://github.com/apache/iggy/pull/2737#discussion_r2819388666
##########
foreign/go/contracts/consumer_groups.go:
##########
@@ -35,30 +35,103 @@ type ConsumerGroupMember struct {
Partitions []uint32
}
-type CreateConsumerGroupRequest struct {
+type CreateConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
Name string `json:"name"`
}
-type DeleteConsumerGroupRequest struct {
+func (c *CreateConsumerGroup) Code() CommandCode {
+ return CreateGroupCode
+}
+
+func (c *CreateConsumerGroup) MarshalBinary() ([]byte, error) {
+ streamIdBytes, err := c.StreamId.MarshalBinary()
+ if err != nil {
+ return nil, err
+ }
+ topicIdBytes, err := c.TopicId.MarshalBinary()
+ if err != nil {
+ return nil, err
+ }
+ offset := len(streamIdBytes) + len(topicIdBytes)
+ bytes := make([]byte, offset+1+len(c.Name))
+ copy(bytes[0:len(streamIdBytes)], streamIdBytes)
+ copy(bytes[len(streamIdBytes):offset], topicIdBytes)
+ bytes[offset] = byte(len(c.Name))
+ copy(bytes[offset+1:], c.Name)
+ return bytes, nil
+}
+
+type DeleteConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
}
-type JoinConsumerGroupRequest struct {
+func (d *DeleteConsumerGroup) Code() CommandCode {
+ return DeleteGroupCode
+}
+
+func (d *DeleteConsumerGroup) MarshalBinary() ([]byte, error) {
+ return marshalIdentifiers(d.StreamId, d.TopicId, d.ConsumerGroupId)
+}
+
+type JoinConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
}
-type LeaveConsumerGroupRequest struct {
+func (j *JoinConsumerGroup) Code() CommandCode {
+ return JoinGroupCode
+}
+
+func (j *JoinConsumerGroup) MarshalBinary() ([]byte, error) {
+ return marshalIdentifiers(j.StreamId, j.TopicId, j.ConsumerGroupId)
+}
+
+type LeaveConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
}
+func (l *LeaveConsumerGroup) Code() CommandCode {
+ return LeaveGroupCode
+}
+
+func (l *LeaveConsumerGroup) MarshalBinary() ([]byte, error) {
+ return marshalIdentifiers(l.StreamId, l.TopicId, l.ConsumerGroupId)
+}
+
+type GetConsumerGroup struct {
+ StreamId Identifier
+ TopicId Identifier
+ GroupId Identifier
Review Comment:
The JSON annotation was left by the previous developers and actually has no use at all. Maybe we can remove them in the future.
That said, I will modify the atribute name for consistency later.
##########
foreign/go/contracts/consumer_groups.go:
##########
@@ -35,30 +35,103 @@ type ConsumerGroupMember struct {
Partitions []uint32
}
-type CreateConsumerGroupRequest struct {
+type CreateConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
Name string `json:"name"`
}
-type DeleteConsumerGroupRequest struct {
+func (c *CreateConsumerGroup) Code() CommandCode {
+ return CreateGroupCode
+}
+
+func (c *CreateConsumerGroup) MarshalBinary() ([]byte, error) {
+ streamIdBytes, err := c.StreamId.MarshalBinary()
+ if err != nil {
+ return nil, err
+ }
+ topicIdBytes, err := c.TopicId.MarshalBinary()
+ if err != nil {
+ return nil, err
+ }
+ offset := len(streamIdBytes) + len(topicIdBytes)
+ bytes := make([]byte, offset+1+len(c.Name))
+ copy(bytes[0:len(streamIdBytes)], streamIdBytes)
+ copy(bytes[len(streamIdBytes):offset], topicIdBytes)
+ bytes[offset] = byte(len(c.Name))
+ copy(bytes[offset+1:], c.Name)
+ return bytes, nil
+}
+
+type DeleteConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
}
-type JoinConsumerGroupRequest struct {
+func (d *DeleteConsumerGroup) Code() CommandCode {
+ return DeleteGroupCode
+}
+
+func (d *DeleteConsumerGroup) MarshalBinary() ([]byte, error) {
+ return marshalIdentifiers(d.StreamId, d.TopicId, d.ConsumerGroupId)
+}
+
+type JoinConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
}
-type LeaveConsumerGroupRequest struct {
+func (j *JoinConsumerGroup) Code() CommandCode {
+ return JoinGroupCode
+}
+
+func (j *JoinConsumerGroup) MarshalBinary() ([]byte, error) {
+ return marshalIdentifiers(j.StreamId, j.TopicId, j.ConsumerGroupId)
+}
+
+type LeaveConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
}
+func (l *LeaveConsumerGroup) Code() CommandCode {
+ return LeaveGroupCode
+}
+
+func (l *LeaveConsumerGroup) MarshalBinary() ([]byte, error) {
+ return marshalIdentifiers(l.StreamId, l.TopicId, l.ConsumerGroupId)
+}
+
+type GetConsumerGroup struct {
+ StreamId Identifier
+ TopicId Identifier
+ GroupId Identifier
Review Comment:
The JSON annotation was left by the previous developers and actually has no use at all. Maybe we can remove them in the future.
That said, I will modify the attribute name for consistency later.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(go): Introduce Command interface to improve Go client request handling [iggy]
Posted by "ex172000 (via GitHub)" <gi...@apache.org>.
ex172000 commented on PR #2737:
URL: https://github.com/apache/iggy/pull/2737#issuecomment-3921699521
@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!
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(go): Introduce Command interface to improve Go client request handling [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #2737:
URL: https://github.com/apache/iggy/pull/2737#discussion_r2819690017
##########
foreign/go/contracts/consumer_groups.go:
##########
@@ -35,30 +35,103 @@ type ConsumerGroupMember struct {
Partitions []uint32
}
-type CreateConsumerGroupRequest struct {
+type CreateConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
Name string `json:"name"`
}
-type DeleteConsumerGroupRequest struct {
+func (c *CreateConsumerGroup) Code() CommandCode {
+ return CreateGroupCode
+}
+
+func (c *CreateConsumerGroup) MarshalBinary() ([]byte, error) {
+ streamIdBytes, err := c.StreamId.MarshalBinary()
+ if err != nil {
+ return nil, err
+ }
+ topicIdBytes, err := c.TopicId.MarshalBinary()
+ if err != nil {
+ return nil, err
+ }
+ offset := len(streamIdBytes) + len(topicIdBytes)
+ bytes := make([]byte, offset+1+len(c.Name))
+ copy(bytes[0:len(streamIdBytes)], streamIdBytes)
+ copy(bytes[len(streamIdBytes):offset], topicIdBytes)
+ bytes[offset] = byte(len(c.Name))
+ copy(bytes[offset+1:], c.Name)
+ return bytes, nil
+}
+
+type DeleteConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
}
-type JoinConsumerGroupRequest struct {
+func (d *DeleteConsumerGroup) Code() CommandCode {
+ return DeleteGroupCode
+}
+
+func (d *DeleteConsumerGroup) MarshalBinary() ([]byte, error) {
+ return marshalIdentifiers(d.StreamId, d.TopicId, d.ConsumerGroupId)
+}
+
+type JoinConsumerGroup struct {
StreamId Identifier `json:"streamId"`
TopicId Identifier `json:"topicId"`
ConsumerGroupId Identifier `json:"consumerGroupId"`
Review Comment:
Regarding the common struct. If I add a common strust like this:
```go
type ConsumerGroupCommandBase struct {
StreamId Identifier
TopicId Identifier
GroupId Identifier
}
```
`GetConsumerGroup` looks like this
```go
type GetConsumerGroup struct {
ConsumerGroupCommandBase
}
```
And when I use it, it would be like:
```go
buffer, err := c.do(&iggcon.GetConsumerGroup{
ConsumerGroupCommandBase: iggcon.ConsumerGroupCommandBase{
StreamId: streamId,
TopicId: topicId,
GroupId: groupId,
},
})
```
To be honest, I prefer the approach without common struct personally, when using it, it looks more straightforward.
```
buffer, err := c.do(&iggcon.GetConsumerGroup{
StreamId: streamId,
TopicId: topicId,
GroupId: groupId,
})
```
Just personal preference. What do you think?
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(go): Introduce Command interface to improve Go client request handling [iggy]
Posted by "ex172000 (via GitHub)" <gi...@apache.org>.
ex172000 commented on PR #2737:
URL: https://github.com/apache/iggy/pull/2737#issuecomment-3922482224
> 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?
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(go): Introduce Command interface to improve Go client request handling [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on PR #2737:
URL: https://github.com/apache/iggy/pull/2737#issuecomment-3918680103
@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?
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(go): Introduce Command interface to improve Go client request handling [iggy]
Posted by "mmodzelewski (via GitHub)" <gi...@apache.org>.
mmodzelewski merged PR #2737:
URL: https://github.com/apache/iggy/pull/2737
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org