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 2025/06/26 06:43:36 UTC
[PR] refactor(sdk): refactor logic for building new Go SDK client using builder pattern and edited some function to align with Rust SDK [iggy]
chengxilo opened a new pull request, #1916:
URL: https://github.com/apache/iggy/pull/1916
# Some explanation
## What I did
1. refactor logic for building new Go SDK client using builder pattern
2. I changed some function to make them align with Rust SDK ( not only their name, I also edited part of their output)
3. I updated the tests.
## Why we need to refactor it.
( I copied the message I sent in discord a few days ago here)
Since currently, the golang sdk is very out dated and when we create a client the
in rust when we create client:
```rust
let client = IggyClientBuilder::new()
.with_tcp()
.with_server_address(get_tcp_server_addr())
.build()?;
```
While in golang we create client:
```go
factory := iggy.IggyClientFactory{}
msgStream, _ := factory.CreateMessageStream(iggcon.IggyConfiguration{
BaseAddress: "vm:8090",
Protocol: "tcp",
})
```
Since the Rust SDK is considered the primary or canonical implementation, it makes sense for other language SDKs to follow its design principles closely — including using the same repository name and similar function names. Clearly, the current Go SDK does not meet this requirement: its function names don’t clearly describe their purpose, and the overall design pattern feels less elegant.
## My solution
So what I did is to read the rust sdk source code and ... almost directly ported its logic to Go. I want to explain why I decided not to use the option pattern discussed earlier with @haze518, even though the option pattern is commonly considered best practice in Go. At first, I choose to follow the example of https://github.com/nats-io/nats.go/blob/main/nats.go#L867 . It's elegent, but it would not work so well in our use case, since we need to support tcp/http/quic.
It's because three different protocol have three different client.
Here is a simplified example showing the potential issue:
```
package main
type Options struct {
protocol string
aTcpOpt bool // a TCP-specific option
aHttpOpt bool // a HTTP-specific option
}
type Option func(*Options)
func WithTcp(opts *Options) {
opts.protocol = "tcp"
}
func WithHttp(opts *Options) {
opts.protocol = "http"
}
func WithATcpOpt(opts *Options) {
opts.aTcpOpt = true
}
func WithAHttpOpt(opts *Options) {
opts.aHttpOpt = true
}
func New(opts ...Option) {
options := &Options{
protocol: "tcp",
aTcpOpt: false,
aHttpOpt: false,
}
for _, opt := range opts {
opt(options)
}
// ... build the client with option
}
func main() {
New(WithTcp, WithAHttpOpt, WithATcpOpt) // user may add a http option when they are using tcp
}
```
So, I think it is not a good choice as I don't know how to solve this problem while using option pattern. But if we use builder pattern, it allows us to control and constrain users’ choices more effectively, preventing invalid combinations and leading to a cleaner, safer API.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2181530658
##########
foreign/go/binary_serialization/binary_request_serializer.go:
##########
@@ -24,32 +24,41 @@ import (
)
func CreateGroup(request CreateConsumerGroupRequest) []byte {
+ if request.ConsumerGroupId == nil {
+ request.ConsumerGroupId = new(uint32)
+ }
customIdOffset := 4 + request.StreamId.Length + request.TopicId.Length
bytes := make([]byte, 4+request.StreamId.Length+request.TopicId.Length+1+4+len(request.Name))
copy(bytes[0:customIdOffset], SerializeIdentifiers(request.StreamId, request.TopicId))
- binary.LittleEndian.PutUint32(bytes[customIdOffset:customIdOffset+4], uint32(request.ConsumerGroupId))
+ binary.LittleEndian.PutUint32(bytes[customIdOffset:customIdOffset+4], *request.ConsumerGroupId)
bytes[customIdOffset+4] = byte(len(request.Name))
- copy(bytes[customIdOffset+5:], []byte(request.Name))
+ copy(bytes[customIdOffset+5:], request.Name)
return bytes
}
-func UpdateOffset(request StoreOffsetRequest) []byte {
+func UpdateOffset(request StoreConsumerOffsetRequest) []byte {
+ if request.PartitionId == nil {
+ request.PartitionId = new(uint32)
+ }
Review Comment:
when the partitionId is nil, assign it with `new(uint32)` i.e. 0. For all of optional arguments like this. When we do the serialization, I give them a default value the same as rust sdk. It's a temporary solution, maybe it should be enhanced in the future as the code doesn't look great.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2181561753
##########
foreign/go/binary_serialization/binary_response_deserializer.go:
##########
@@ -62,29 +71,7 @@ func DeserializeStreams(payload []byte) []StreamResponse {
return streams
Review Comment:
I add TODO here, since this pr is not aimed to solve all of problems like this. We can do this in the future, I added a few TODOs like this.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2181574775
##########
foreign/go/binary_serialization/binary_response_deserializer.go:
##########
@@ -28,27 +28,36 @@ import (
"github.com/klauspost/compress/s2"
)
-func DeserializeLogInResponse(payload []byte) *LogInResponse {
+func DeserializeLogInResponse(payload []byte) *IdentityInfo {
userId := binary.LittleEndian.Uint32(payload[0:4])
- return &LogInResponse{
+ return &IdentityInfo{
UserId: userId,
}
}
-func DeserializeOffset(payload []byte) *OffsetResponse {
+func DeserializeOffset(payload []byte) *ConsumerOffsetInfo {
partitionId := int(binary.LittleEndian.Uint32(payload[0:4]))
currentOffset := binary.LittleEndian.Uint64(payload[4:12])
storedOffset := binary.LittleEndian.Uint64(payload[12:20])
- return &OffsetResponse{
+ return &ConsumerOffsetInfo{
PartitionId: partitionId,
CurrentOffset: currentOffset,
StoredOffset: storedOffset,
}
}
-func DeserializeStreams(payload []byte) []StreamResponse {
- streams := make([]StreamResponse, 0)
+func DeserializeStream(payload []byte) *StreamDetails {
+ stream, _ := DeserializeToStream(payload, 0)
+ // TODO implement deserialize topics
Review Comment:
I add TODO here, as the StreamDetails need a topic slice. Since this pr is not aimed to solve all of problems like this. We can do this in the future. Also, since this pr already has 1600 lines changes, I can not imagine how many lines would be changed if I add this new logic and add some tests.
I added a few TODOs like this.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2181574775
##########
foreign/go/binary_serialization/binary_response_deserializer.go:
##########
@@ -28,27 +28,36 @@ import (
"github.com/klauspost/compress/s2"
)
-func DeserializeLogInResponse(payload []byte) *LogInResponse {
+func DeserializeLogInResponse(payload []byte) *IdentityInfo {
userId := binary.LittleEndian.Uint32(payload[0:4])
- return &LogInResponse{
+ return &IdentityInfo{
UserId: userId,
}
}
-func DeserializeOffset(payload []byte) *OffsetResponse {
+func DeserializeOffset(payload []byte) *ConsumerOffsetInfo {
partitionId := int(binary.LittleEndian.Uint32(payload[0:4]))
currentOffset := binary.LittleEndian.Uint64(payload[4:12])
storedOffset := binary.LittleEndian.Uint64(payload[12:20])
- return &OffsetResponse{
+ return &ConsumerOffsetInfo{
PartitionId: partitionId,
CurrentOffset: currentOffset,
StoredOffset: storedOffset,
}
}
-func DeserializeStreams(payload []byte) []StreamResponse {
- streams := make([]StreamResponse, 0)
+func DeserializeStream(payload []byte) *StreamDetails {
+ stream, _ := DeserializeToStream(payload, 0)
+ // TODO implement deserialize topics
Review Comment:
I add TODO here, as the StreamDetails need a topic slice. Since this pr is not aimed to solve all of problems like this. We can do this in the future, I added a few TODOs like this.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2181550368
##########
foreign/go/binary_serialization/binary_response_deserializer.go:
##########
@@ -107,9 +94,9 @@ func DeserializeToStream(payload []byte, position int) (StreamResponse, int) {
}, readBytes
}
Review Comment:
`DeserializeStream` deserialize the payload according to the position, to a `StreamDetails`(has more attribute than Stream)
`DeserializeToStream` deserialize the payload to `Stream`, and return the position after reading.
`DeserializeStreams` deserialize the payload to a `Stream` slice.
So basically `DeserializeToStream` is used as the tool for `DeserializeStream` and `DeserializeStreams`. You will see a lot of function like this in the serialization layor. I moidfy my code according to this pattern.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on PR #1916:
URL: https://github.com/apache/iggy/pull/1916#issuecomment-3030376619
All function signatures and return types have been updated. Required arguments are passed directly, while optional arguments are represented as pointer values. I try to limit the scope of changes in this PR to function signatures and their usage logic, as this pr is already too huge. However, I had to modify some functions—mainly in the serialization layer—to align with the updated argument and return value structure. For more detail. Please check the detail I provided in review.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "spetz (via GitHub)" <gi...@apache.org>.
spetz merged PR #1916:
URL: https://github.com/apache/iggy/pull/1916
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2181588140
##########
foreign/go/binary_serialization/log_in_request_serializer.go:
##########
@@ -19,19 +19,18 @@ package binaryserialization
import (
"encoding/binary"
-
- iggcon "github.com/apache/iggy/foreign/go/contracts"
)
type TcpLogInRequest struct {
- iggcon.LogInRequest
+ Username string `json:"username"`
+ Password string `json:"password"`
}
func (request *TcpLogInRequest) Serialize() []byte {
usernameBytes := []byte(request.Username)
passwordBytes := []byte(request.Password)
- versionBytes := []byte(request.Version)
- contextBytes := []byte(request.Context)
+ versionBytes := []byte("")
+ contextBytes := []byte("")
Review Comment:
I use "", "" to replace them, currently it would have the same behavior as the previous golang sdk. In rust sdk, they will read the cargo pkg version. I think it doesn't work for go, and I am not sure whether it is some historical code.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2181561753
##########
foreign/go/binary_serialization/binary_response_deserializer.go:
##########
@@ -62,29 +71,7 @@ func DeserializeStreams(payload []byte) []StreamResponse {
return streams
Review Comment:
I add TODO here, since this pr is not aimed to solve all of problems like this. We can do this in the future, I added a few TODOs like this.
--
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(sdk): refactor logic for building new Go SDK client using builder pattern and edited some function to align with Rust SDK [iggy]
Posted by "haze518 (via GitHub)" <gi...@apache.org>.
haze518 commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2173265154
##########
foreign/go/samples/consumer/consumer.go:
##########
@@ -38,39 +38,32 @@ const (
)
func main() {
- factory := &IggyClientFactory{}
- config := IggyConfiguration{
- BaseAddress: "127.0.0.1:8090",
- Protocol: Tcp,
- }
-
- messageStream, err := factory.CreateMessageStream(config)
+ cli, err := iggycli.NewIggyClientBuilder().WithTcp().WithServerAddress("127.0.0.1:8090").Build()
if err != nil {
panic(err)
}
-
- _, err = messageStream.LogIn(LogInRequest{
+ _, err = cli.LoginUser(LoginUserRequest{
Username: "iggy",
Password: "iggy",
})
if err != nil {
panic("COULD NOT LOG IN")
}
- if err = EnsureInsfrastructureIsInitialized(messageStream); err != nil {
+ if err = EnsureInsfrastructureIsInitialized(cli); err != nil {
panic(err)
}
- if err := ConsumeMessages(messageStream); err != nil {
+ if err := ConsumeMessages(*cli); err != nil {
Review Comment:
here you don’t need to dereference, since ConsumeMessages expects an interface, so you can just pass a pointer
##########
foreign/go/iggycli/iggy_client.go:
##########
@@ -0,0 +1,193 @@
+// 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 iggycli
+
+import (
+ . "github.com/apache/iggy/foreign/go/contracts"
+ ierror "github.com/apache/iggy/foreign/go/errors"
+)
+
+type IggyClient struct {
Review Comment:
I don’t quite get it — why do we need this wrapper around the client? It seems like it doesn’t really do much, except just forward the call to the client’s method
##########
foreign/go/iggycli/client_builder.go:
##########
@@ -0,0 +1,93 @@
+// 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 iggycli
+
+import (
+ "context"
+ "fmt"
+
+ ierror "github.com/apache/iggy/foreign/go/errors"
+ "github.com/apache/iggy/foreign/go/tcp"
+)
+
+type IggyClientBuilder struct {
+ client Client
+}
+
+// NewIggyClientBuilder create a new IggyClientBuilder
+// This is not enough to build the IggyClient instance. You need to provide the client configuration or the client
+// implementation for the specific transport.
+func NewIggyClientBuilder() *IggyClientBuilder {
Review Comment:
What do you think about this approach (yeah, it’s me with my options again 😄)?
If we created an enum to determine which transport the user wants to use, and also added With... options for each transport’s config, we’d end up with something like this:
```go
// client_builder.go
type Transport int
const (
TransportTCP Transport = iota
)
type Config struct {
transport Transport
tcpConfig *tcp.ClientConfig
}
func NewDefaultConfig() *Config {
return &Config{
transport: TransportTCP,
tcpConfig: NewDefaultTCPConfig(),
}
}
type Option func(*Config)
func WithTCP(tcpOpts ...TCPOption) Option {
return func(c *Config) {
c.transport = TransportTCP
tcp := NewDefaultTCPConfig()
for _, o := range tcpOpts {
o(tcp)
}
c.tcpConfig = tcp
}
}
func New(options ...Option) (Client, error) {
cfg := NewDefaultConfig()
for _, o := range options {
o(cfg)
}
switch cfg.transport {
case TransportTCP:
cli, err := tcp.NewIggyTcpClient(*cfg.tcpConfig)
if err != nil {
return nil, err
}
return cli, nil
default:
return nil, fmt.Errorf("unknown transport type: %d", cfg.transport)
}
}
// tcp_core.go
type ClientConfig struct {
Ctx context.Context
ServerAddress string
HeartbeatInterval time.Duration
}
type TCPOption func(*ClientConfig)
func WithServerAddress(address string) TCPOption {
return func(c *ClientConfig) {
c.ServerAddress = address
}
}
```
Honestly, I just like this approach better — compared to a builder, there’s way less boilerplate (no need for extra chains or factories), and it feels much easier to expand in the future. Also, it’s more idiomatic for go, at least in my opinion (even if that’s up for debate).
But I’d be interested to hear your thoughts on this
--
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(sdk): refactor logic for building new Go SDK client using builder pattern and edited some function to align with Rust SDK [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2173379084
##########
foreign/go/samples/consumer/consumer.go:
##########
@@ -38,39 +38,32 @@ const (
)
func main() {
- factory := &IggyClientFactory{}
- config := IggyConfiguration{
- BaseAddress: "127.0.0.1:8090",
- Protocol: Tcp,
- }
-
- messageStream, err := factory.CreateMessageStream(config)
+ cli, err := iggycli.NewIggyClientBuilder().WithTcp().WithServerAddress("127.0.0.1:8090").Build()
if err != nil {
panic(err)
}
-
- _, err = messageStream.LogIn(LogInRequest{
+ _, err = cli.LoginUser(LoginUserRequest{
Username: "iggy",
Password: "iggy",
})
if err != nil {
panic("COULD NOT LOG IN")
}
- if err = EnsureInsfrastructureIsInitialized(messageStream); err != nil {
+ if err = EnsureInsfrastructureIsInitialized(cli); err != nil {
panic(err)
}
- if err := ConsumeMessages(messageStream); err != nil {
+ if err := ConsumeMessages(*cli); err != nil {
Review Comment:
It's because `func ConsumeMessages(messageStream iggycli.IggyClient) error` in `foreign/go/samples/consumer/consumer.go` only accept iggycli.IggyClient instead of *iggycli.IggyClient. I will change this function instead of dereference 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(sdk): refactor logic for building new Go SDK client using builder pattern and edited some function to align with Rust SDK [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2173379084
##########
foreign/go/samples/consumer/consumer.go:
##########
@@ -38,39 +38,32 @@ const (
)
func main() {
- factory := &IggyClientFactory{}
- config := IggyConfiguration{
- BaseAddress: "127.0.0.1:8090",
- Protocol: Tcp,
- }
-
- messageStream, err := factory.CreateMessageStream(config)
+ cli, err := iggycli.NewIggyClientBuilder().WithTcp().WithServerAddress("127.0.0.1:8090").Build()
if err != nil {
panic(err)
}
-
- _, err = messageStream.LogIn(LogInRequest{
+ _, err = cli.LoginUser(LoginUserRequest{
Username: "iggy",
Password: "iggy",
})
if err != nil {
panic("COULD NOT LOG IN")
}
- if err = EnsureInsfrastructureIsInitialized(messageStream); err != nil {
+ if err = EnsureInsfrastructureIsInitialized(cli); err != nil {
panic(err)
}
- if err := ConsumeMessages(messageStream); err != nil {
+ if err := ConsumeMessages(*cli); err != nil {
Review Comment:
I will fix it.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "haze518 (via GitHub)" <gi...@apache.org>.
haze518 commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2183975044
##########
foreign/go/binary_serialization/log_in_request_serializer.go:
##########
@@ -19,19 +19,18 @@ package binaryserialization
import (
"encoding/binary"
-
- iggcon "github.com/apache/iggy/foreign/go/contracts"
)
type TcpLogInRequest struct {
- iggcon.LogInRequest
+ Username string `json:"username"`
+ Password string `json:"password"`
}
func (request *TcpLogInRequest) Serialize() []byte {
usernameBytes := []byte(request.Username)
passwordBytes := []byte(request.Password)
- versionBytes := []byte(request.Version)
- contextBytes := []byte(request.Context)
+ versionBytes := []byte("")
+ contextBytes := []byte("")
Review Comment:
yeah, actually, first of all it looks like this is legacy, and secondly, there isn’t really an equivalent in Go—unless you inject the version via ldflags (which might be overkill), or just use the tag number
--
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(sdk): refactor logic for building new Go SDK client using builder pattern and edited some function to align with Rust SDK [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2173401363
##########
foreign/go/iggycli/client_builder.go:
##########
@@ -0,0 +1,93 @@
+// 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 iggycli
+
+import (
+ "context"
+ "fmt"
+
+ ierror "github.com/apache/iggy/foreign/go/errors"
+ "github.com/apache/iggy/foreign/go/tcp"
+)
+
+type IggyClientBuilder struct {
+ client Client
+}
+
+// NewIggyClientBuilder create a new IggyClientBuilder
+// This is not enough to build the IggyClient instance. You need to provide the client configuration or the client
+// implementation for the specific transport.
+func NewIggyClientBuilder() *IggyClientBuilder {
Review Comment:
It seems like you're a big fan of functional programming, hahaha.😂 Why I didn't come up with this idea! I think it is a amazing solution for this problem. I will modify my code according to your suggestion. Thank you so much.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2181600461
##########
foreign/go/contracts/consumer.go:
##########
@@ -20,8 +20,8 @@ package iggcon
type ConsumerKind int
const (
- ConsumerSingle ConsumerKind = 1
- ConsumerGroup ConsumerKind = 2
+ ConsumerKindSingle ConsumerKind = 1
+ ConsumerKindGroup ConsumerKind = 2
Review Comment:
I rename it, because name `ConsumerGroup` would be used as a type.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2181501936
##########
bdd/go/tests/tcp_test/users_steps.go:
##########
@@ -140,35 +137,32 @@ func itShouldSuccessfullyUpdateUserPermissions(userId uint32, client iggy.Messag
}
func itShouldBePossibleToLogInWithCredentials(username string, password string) {
- ms := createConnection()
- userId, err := ms.LogIn(iggcon.LogInRequest{
- Username: username,
- Password: password,
- })
+ ms := createClient()
+ userId, err := ms.LoginUser(username, password)
itShouldNotReturnError(err)
It("should return userId", func() {
Expect(userId).NotTo(BeNil())
})
}
-func itShouldReturnSpecificUser(name string, user iggcon.UserResponse) {
+func itShouldReturnSpecificUser(name string, user iggcon.UserInfo) {
It("should fetch user with name "+name, func() {
Expect(user.Username).To(Equal(name))
})
}
-func itShouldContainSpecificUser(name string, users []*iggcon.UserResponse) {
+func itShouldContainSpecificUser(name string, users []iggcon.UserInfo) {
Review Comment:
In this update, I changed `[]*iggcon.UserResponse` to `[]iggcon.UserInfo`. Although both are slices, the new version uses value elements instead of pointers. I made this change because UserInfo is relatively lightweight, and for consistency—all other functions returning slices use values rather than pointers.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2181530658
##########
foreign/go/binary_serialization/binary_request_serializer.go:
##########
@@ -24,32 +24,41 @@ import (
)
func CreateGroup(request CreateConsumerGroupRequest) []byte {
+ if request.ConsumerGroupId == nil {
+ request.ConsumerGroupId = new(uint32)
+ }
customIdOffset := 4 + request.StreamId.Length + request.TopicId.Length
bytes := make([]byte, 4+request.StreamId.Length+request.TopicId.Length+1+4+len(request.Name))
copy(bytes[0:customIdOffset], SerializeIdentifiers(request.StreamId, request.TopicId))
- binary.LittleEndian.PutUint32(bytes[customIdOffset:customIdOffset+4], uint32(request.ConsumerGroupId))
+ binary.LittleEndian.PutUint32(bytes[customIdOffset:customIdOffset+4], *request.ConsumerGroupId)
bytes[customIdOffset+4] = byte(len(request.Name))
- copy(bytes[customIdOffset+5:], []byte(request.Name))
+ copy(bytes[customIdOffset+5:], request.Name)
return bytes
}
-func UpdateOffset(request StoreOffsetRequest) []byte {
+func UpdateOffset(request StoreConsumerOffsetRequest) []byte {
+ if request.PartitionId == nil {
+ request.PartitionId = new(uint32)
+ }
Review Comment:
when the partitionId is nil, assign it with `new(uint32)` i.e. 0. For all of these optional value. When we do the serialization, I give them a default value the same as rust sdk.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2181550368
##########
foreign/go/binary_serialization/binary_response_deserializer.go:
##########
@@ -107,9 +94,9 @@ func DeserializeToStream(payload []byte, position int) (StreamResponse, int) {
}, readBytes
}
Review Comment:
`DeserializeStream` deserializes the payload starting from a given position into a `StreamDetails` object, which contains more attributes than `Stream`.
`DeserializeToStream` deserializes the payload into a `Stream` and returns the new position after reading.
`DeserializeStreams` deserializes the payload into a slice of `Stream` objects.
Essentially, `DeserializeToStream` serves as a helper for both `DeserializeStream` and `DeserializeStreams`. This kind of layered deserialization pattern is common in the serialization layer, and I modified my code to follow this established approach.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2181561753
##########
foreign/go/binary_serialization/binary_response_deserializer.go:
##########
@@ -62,29 +71,7 @@ func DeserializeStreams(payload []byte) []StreamResponse {
return streams
Review Comment:
I add TODO here, since this pr is not aimed to solve all of problems like this. We can do this in the future. There are a few TODOs and all of them are added for this purpose.
--
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(sdk): refactor logic for building new Go SDK client using builder pattern and edited some function to align with Rust SDK [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2173387923
##########
foreign/go/iggycli/iggy_client.go:
##########
@@ -0,0 +1,193 @@
+// 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 iggycli
+
+import (
+ . "github.com/apache/iggy/foreign/go/contracts"
+ ierror "github.com/apache/iggy/foreign/go/errors"
+)
+
+type IggyClient struct {
Review Comment:
Oh for this part, actually I was creating the builder according to rust source code.
In the rust source code we have IggyClient to wrap the tcp/quic/http client.
https://github.com/apache/iggy/blob/934cf1c36435535cb12470fef381e3a5af824ad3/core/sdk/src/clients/client.rs#L47-L51
So I want to use the similar structure to make it more convinient to add encryptor and partitioner in the future. Maybe I need to add some comment to this part to show that I keep this wrapper intentionally?
--
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(sdk): refactor logic for building new Go SDK client using builder pattern and edited some function to align with Rust SDK [iggy]
Posted by "haze518 (via GitHub)" <gi...@apache.org>.
haze518 commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2173574855
##########
foreign/go/iggycli/iggy_client.go:
##########
@@ -0,0 +1,193 @@
+// 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 iggycli
+
+import (
+ . "github.com/apache/iggy/foreign/go/contracts"
+ ierror "github.com/apache/iggy/foreign/go/errors"
+)
+
+type IggyClient struct {
Review Comment:
I’ve moved this discussion to discord (https://discord.com/channels/1144142576266530928/1144143119542128710/1388703413860176014), just in case, to get some feedback from the other guys as well
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2181550368
##########
foreign/go/binary_serialization/binary_response_deserializer.go:
##########
@@ -107,9 +94,9 @@ func DeserializeToStream(payload []byte, position int) (StreamResponse, int) {
}, readBytes
}
Review Comment:
`DeserializeStream` deserialize the payload according to the position, to a `StreamDetails`(has more attribute than Stream)
`DeserializeToStream` deserialize the payload to stream, and return the position after reading.
`DeserializeStreams` deserialize the payload to a `Stream` slice.
So basically `DeserializeToStream` is used as the tool for `DeserializeStream` and `DeserializeStreams`. You will see a lot of function like this in the serialization layor. I moidfy my code according to this pattern.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2181574775
##########
foreign/go/binary_serialization/binary_response_deserializer.go:
##########
@@ -28,27 +28,36 @@ import (
"github.com/klauspost/compress/s2"
)
-func DeserializeLogInResponse(payload []byte) *LogInResponse {
+func DeserializeLogInResponse(payload []byte) *IdentityInfo {
userId := binary.LittleEndian.Uint32(payload[0:4])
- return &LogInResponse{
+ return &IdentityInfo{
UserId: userId,
}
}
-func DeserializeOffset(payload []byte) *OffsetResponse {
+func DeserializeOffset(payload []byte) *ConsumerOffsetInfo {
partitionId := int(binary.LittleEndian.Uint32(payload[0:4]))
currentOffset := binary.LittleEndian.Uint64(payload[4:12])
storedOffset := binary.LittleEndian.Uint64(payload[12:20])
- return &OffsetResponse{
+ return &ConsumerOffsetInfo{
PartitionId: partitionId,
CurrentOffset: currentOffset,
StoredOffset: storedOffset,
}
}
-func DeserializeStreams(payload []byte) []StreamResponse {
- streams := make([]StreamResponse, 0)
+func DeserializeStream(payload []byte) *StreamDetails {
+ stream, _ := DeserializeToStream(payload, 0)
+ // TODO implement deserialize topics
Review Comment:
I add TODO here, as the StreamDetails need a topic slice. Since this pr is not aimed to solve all of problems like this. We can do this in the future.
I added a few TODOs like this.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2181501936
##########
bdd/go/tests/tcp_test/users_steps.go:
##########
@@ -140,35 +137,32 @@ func itShouldSuccessfullyUpdateUserPermissions(userId uint32, client iggy.Messag
}
func itShouldBePossibleToLogInWithCredentials(username string, password string) {
- ms := createConnection()
- userId, err := ms.LogIn(iggcon.LogInRequest{
- Username: username,
- Password: password,
- })
+ ms := createClient()
+ userId, err := ms.LoginUser(username, password)
itShouldNotReturnError(err)
It("should return userId", func() {
Expect(userId).NotTo(BeNil())
})
}
-func itShouldReturnSpecificUser(name string, user iggcon.UserResponse) {
+func itShouldReturnSpecificUser(name string, user iggcon.UserInfo) {
It("should fetch user with name "+name, func() {
Expect(user.Username).To(Equal(name))
})
}
-func itShouldContainSpecificUser(name string, users []*iggcon.UserResponse) {
+func itShouldContainSpecificUser(name string, users []iggcon.UserInfo) {
Review Comment:
In this update, I changed `[]*iggcon.UserResponse` to `[]iggcon.UserInfo`. Although both are slices, the new version uses value elements instead of pointers. I made this change because UserInfo is relatively lightweight, and for consistency—all other functions returning slices use values rather than pointers.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2181530658
##########
foreign/go/binary_serialization/binary_request_serializer.go:
##########
@@ -24,32 +24,41 @@ import (
)
func CreateGroup(request CreateConsumerGroupRequest) []byte {
+ if request.ConsumerGroupId == nil {
+ request.ConsumerGroupId = new(uint32)
+ }
customIdOffset := 4 + request.StreamId.Length + request.TopicId.Length
bytes := make([]byte, 4+request.StreamId.Length+request.TopicId.Length+1+4+len(request.Name))
copy(bytes[0:customIdOffset], SerializeIdentifiers(request.StreamId, request.TopicId))
- binary.LittleEndian.PutUint32(bytes[customIdOffset:customIdOffset+4], uint32(request.ConsumerGroupId))
+ binary.LittleEndian.PutUint32(bytes[customIdOffset:customIdOffset+4], *request.ConsumerGroupId)
bytes[customIdOffset+4] = byte(len(request.Name))
- copy(bytes[customIdOffset+5:], []byte(request.Name))
+ copy(bytes[customIdOffset+5:], request.Name)
return bytes
}
-func UpdateOffset(request StoreOffsetRequest) []byte {
+func UpdateOffset(request StoreConsumerOffsetRequest) []byte {
+ if request.PartitionId == nil {
+ request.PartitionId = new(uint32)
+ }
Review Comment:
when the partitionId is nil, assign it with `new(uint32)` i.e. 0. For all of optional arguments like this. When we do the serialization, I give them a default value the same as rust sdk.
--
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(sdk): refactor Go SDK client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #1916:
URL: https://github.com/apache/iggy/pull/1916#discussion_r2181603571
##########
foreign/go/tcp/tcp_user_managament.go:
##########
@@ -36,7 +36,7 @@ func (tms *IggyTcpClient) GetUser(identifier Identifier) (*UserResponse, error)
return binaryserialization.DeserializeUser(buffer)
}
-func (tms *IggyTcpClient) GetUsers() ([]*UserResponse, error) {
+func (tms *IggyTcpClient) GetUsers() ([]UserInfo, error) {
Review Comment:
In this update, I changed []*UserResponse to []iggcon.UserInfo. Although both are slices, the new version uses value elements instead of pointers. I made this change because UserInfo is relatively lightweight, and for consistency—all other functions returning slices use values rather than pointers.
--
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