You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iggy.apache.org by "explise (via GitHub)" <gi...@apache.org> on 2026/07/06 21:16:44 UTC
[PR] feat(go): add diagnostic event subscriptions to TCP client [iggy]
explise opened a new pull request, #3621:
URL: https://github.com/apache/iggy/pull/3621
## Which issue does this PR address
Closes #3219
## Rationale
The TCP client had `// TODO publish event` stubs at the connect/disconnect/
shutdown paths with no way for callers to observe connection or auth state
changes. This fills those in and is groundwork for a high-level `IggyConsumer`
abstraction: diagnostic events let the consumer detect disconnects and
re-join consumer groups eagerly on reconnect. They're also useful on their own
for application-level observability (logging drops, health metrics).
## What changed?
- `DiagnosticEvent` enum + `String()` in `contracts/`, and `SubscribeEvents()`
on the `Client` interface.
- A small non-blocking broadcaster in the TCP client fanning out to multiple
independent subscribers.
- Emission wired into connect (`Connected`), disconnect/failed-connect
(`Disconnected`), shutdown (`Shutdown`), login (`SignedIn`) and logout
(`SignedOut`).
## Local Execution
- **Passed.** `go build ./...`, `go vet ./...`, `gofmt`, and `go mod tidy` are clean.
- Unit tests pass: `DiagnosticEvent.String`, and the broadcaster tests
(fan-out / ordering / slow-subscriber-drop / idempotent-close).
- Integration tests pass — all 5 in `tests/diagnostic_events_test.go`
(login → SignedIn, logout → SignedOut, close → Shutdown + channel close,
multiple subscribers, and the full connect → login → logout → close sequence),
run against a real `apache/iggy:edge` container via testcontainers.
- **Pre-commit hooks ran** via `prek` (pre-commit + pre-push), including
`golangci-lint` on `foreign/go` — passed.
## AI Usage
1. **Tool:** Claude Code.
2. **Scope:** Implementation of the enum, broadcaster, and event wiring;
rebase/conflict resolution onto latest `master`; test authoring.
3. **Verification:** Local `go build`/`go vet`/unit tests, `golangci-lint`, and
manual review of every changed line against the surrounding code.
4. **Can I explain every line?** Yes.
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3545522300
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -41,6 +41,7 @@ func (c *IggyTcpClient) LoginUser(ctx context.Context, username string, password
c.logger.Info("Iggy client has signed in successfully.", slog.String("client_address", c.clientAddress))
identity := binaryserialization.DeserializeLogInResponse(buffer)
+ c.events.publish(iggcon.DiagnosticEventSignedIn)
Review Comment:
@explise Your concern is correst, I didn't thought it throughly. Technically, because the redirect needs to call `GetClusterMetadata`, a session is required. It's true that we will log in every time before we redirect, so keeping the current approach might be the best choice(publish the login event multiple times, since it's true that we logined). While they are noise, they tell the subscriber these events actually happened.
So it may be better to keep this `publish_event...`, ignore my suggestion to move this to the end of SignIn function.
But I still believe it might be better to add the redirect event. I checked the code of IggyProducer and IggyConsumer, if the redirect event is provided, it would help it to filter the noise during redirection. For example, when received the redirect event, ignore the following `sign in` ,`disconnect`, `connect`. @hubcio would that help?
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3545529884
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
Review Comment:
This one can be ignored.
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3576852947
##########
foreign/go/client/tcp/tcp_core.go:
##########
Review Comment:
Will be addressed in #3670, ignore it.
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -624,7 +631,7 @@ func (c *IggyTcpClient) disconnect() error {
}
Review Comment:
Will be addressed in #3670, ignore 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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3545522300
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -41,6 +41,7 @@ func (c *IggyTcpClient) LoginUser(ctx context.Context, username string, password
c.logger.Info("Iggy client has signed in successfully.", slog.String("client_address", c.clientAddress))
identity := binaryserialization.DeserializeLogInResponse(buffer)
+ c.events.publish(iggcon.DiagnosticEventSignedIn)
Review Comment:
Technically, because the redirect needs to call `GetClusterMetadata`, a session is required. It's true that we will log in every time before we redirect, so keeping the current approach might be the best choice(publish the login event multiple times, since it's true that we logined). However, I think it is still worth adding a redirect event. Maybe it's something we can add to Rust sdk too.
My recommendation would be keep this `publish_event...`, ignore my suggestion to move this to the end of SignIn function. But add the publish redirect event logic in the HandleLeaderRedirection functions.
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on PR #3621:
URL: https://github.com/apache/iggy/pull/3621#issuecomment-4972631481
when resolve conflict, replace
```go
if c.conn != nil {
if err := c.conn.Close(); err != nil {
return err
}
}
```
with
```go
err := c.closeConnLocked()
```
I hope this would help.
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3542916693
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -41,6 +41,7 @@ func (c *IggyTcpClient) LoginUser(ctx context.Context, username string, password
c.logger.Info("Iggy client has signed in successfully.", slog.String("client_address", c.clientAddress))
identity := binaryserialization.DeserializeLogInResponse(buffer)
+ c.events.publish(iggcon.DiagnosticEventSignedIn)
Review Comment:
unfortunately my golang knowledge is second to none, sorry. @chengxilo any thoughts on above question?
from Rust perspective, feel free to break parity.
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3545522300
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -41,6 +41,7 @@ func (c *IggyTcpClient) LoginUser(ctx context.Context, username string, password
c.logger.Info("Iggy client has signed in successfully.", slog.String("client_address", c.clientAddress))
identity := binaryserialization.DeserializeLogInResponse(buffer)
+ c.events.publish(iggcon.DiagnosticEventSignedIn)
Review Comment:
@explise Your concern is correst, I didn't thought it throughly. Technically, because the redirect needs to call `GetClusterMetadata`, a session is required. It's true that we will log in every time before we redirect, so keeping the current approach might be the best choice(publish the login event multiple times, since it's true that we logined).
So it may be better to keep this `publish_event...`, ignore my suggestion to move this to the end of SignIn function. But add the publish redirect event logic in the HandleLeaderRedirection functions.
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3545522300
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -41,6 +41,7 @@ func (c *IggyTcpClient) LoginUser(ctx context.Context, username string, password
c.logger.Info("Iggy client has signed in successfully.", slog.String("client_address", c.clientAddress))
identity := binaryserialization.DeserializeLogInResponse(buffer)
+ c.events.publish(iggcon.DiagnosticEventSignedIn)
Review Comment:
@explise Your concern is correst, I didn't thought it throughly. Technically, because the redirect needs to call `GetClusterMetadata`, a session is required. It's true that we will log in every time before we redirect, so keeping the current approach might be the best choice(publish the login event multiple times, since it's true that we logined).
So it may be better to keep this `publish_event...`, ignore my suggestion to move this to the end of SignIn function.
But I still believe it might be better to add the redirect event. I checked the code of IggyProducer and IggyConsumer, if the redirect event is provided, it would help it to filter the noise during redirection. For example, when received the redirect event, ignore the following sign in ,connect, disconnect
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3545522300
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -41,6 +41,7 @@ func (c *IggyTcpClient) LoginUser(ctx context.Context, username string, password
c.logger.Info("Iggy client has signed in successfully.", slog.String("client_address", c.clientAddress))
identity := binaryserialization.DeserializeLogInResponse(buffer)
+ c.events.publish(iggcon.DiagnosticEventSignedIn)
Review Comment:
@explise Your concern is correst, I didn't thought it throughly. Technically, because the redirect needs to call `GetClusterMetadata`, a session is required. It's true that we will log in every time before we redirect, so keeping the current approach might be the best choice(publish the login event multiple times, since it's true that we logined). While they are noise, they tell the subscriber these events actually happened.
So it may be better to keep this `publish_event...`, ignore my suggestion to move this to the end of SignIn function.
But I still believe it might be better to add the redirect event. I checked the code of IggyProducer and IggyConsumer, if the redirect event is provided, it would help it to filter the noise during redirection. For example, when received the redirect event, ignore the following `disconnect`, `connect`,`disconnect`. @hubcio would that help?
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3621:
URL: https://github.com/apache/iggy/pull/3621#issuecomment-4897688715
<!-- iggy-pr-triage-welcome -->
Thanks for the PR. It is labeled `S-waiting-on-review` and queued for review.
Slash commands (own line, regular comment) move it around the queue:
- `/ready` - back to `S-waiting-on-review` after addressing feedback
- `/author` - flip to `S-waiting-on-author` while you finish changes
- `/request-review @user-or-team` - request a reviewer
See [CONTRIBUTING.md](https://github.com/apache/iggy/blob/master/CONTRIBUTING.md#pr-triage-commands) for details.
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on PR #3621:
URL: https://github.com/apache/iggy/pull/3621#issuecomment-4961354717
/ready
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "ryankert01 (via GitHub)" <gi...@apache.org>.
ryankert01 commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3556701272
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -431,12 +433,16 @@ func (c *IggyTcpClient) sendLocked(wirePayload []byte) ([]byte, error) {
return buffer, nil
}
-// invalidateConnLocked closes the connection and marks it as disconnected
+// invalidateConnLocked closes the connection and marks it as disconnected,
+// publishing a Disconnected event on the transition.
func (c *IggyTcpClient) invalidateConnLocked() {
if c.conn != nil {
_ = c.conn.Close()
}
- c.state = iggcon.StateDisconnected
+ if c.state != iggcon.StateDisconnected {
+ c.state = iggcon.StateDisconnected
+ c.events.publish(iggcon.DiagnosticEventDisconnected)
Review Comment:
newTestClient create but will not init it, so it will failed.
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3575951108
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -624,7 +631,7 @@ func (c *IggyTcpClient) disconnect() error {
}
Review Comment:
```go
var err error
if c.conn != nil {
err = c.conn.Close()
}
```
This part on hold, I need to consider about it. (Same as above)
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3576850748
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -431,12 +433,16 @@ func (c *IggyTcpClient) sendLocked(wirePayload []byte) ([]byte, error) {
return buffer, nil
}
-// invalidateConnLocked closes the connection and marks it as disconnected
+// invalidateConnLocked closes the connection and marks it as disconnected,
+// publishing a Disconnected event on the transition.
func (c *IggyTcpClient) invalidateConnLocked() {
if c.conn != nil {
_ = c.conn.Close()
Review Comment:
Will be addressed in #3670, ignore 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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "explise (via GitHub)" <gi...@apache.org>.
explise commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3570334021
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -431,12 +433,16 @@ func (c *IggyTcpClient) sendLocked(wirePayload []byte) ([]byte, error) {
return buffer, nil
}
-// invalidateConnLocked closes the connection and marks it as disconnected
+// invalidateConnLocked closes the connection and marks it as disconnected,
+// publishing a Disconnected event on the transition.
func (c *IggyTcpClient) invalidateConnLocked() {
if c.conn != nil {
_ = c.conn.Close()
}
- c.state = iggcon.StateDisconnected
+ if c.state != iggcon.StateDisconnected {
+ c.state = iggcon.StateDisconnected
+ c.events.publish(iggcon.DiagnosticEventDisconnected)
Review Comment:
i have resolved it, can u check again please
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3545522300
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -41,6 +41,7 @@ func (c *IggyTcpClient) LoginUser(ctx context.Context, username string, password
c.logger.Info("Iggy client has signed in successfully.", slog.String("client_address", c.clientAddress))
identity := binaryserialization.DeserializeLogInResponse(buffer)
+ c.events.publish(iggcon.DiagnosticEventSignedIn)
Review Comment:
@explise Your concern is correst, I didn't thought it throughly. Technically, because the redirect needs to call `GetClusterMetadata`, a session is required. It's true that we will log in every time before we redirect, so keeping the current approach might be the best choice(publish the login event multiple times, since it's true that we logined). However, I think it is still worth adding a redirect event. Maybe it's something we can add to Rust sdk too.
My recommendation would be keep this `publish_event...`, ignore my suggestion to move this to the end of SignIn function. But add the publish redirect event logic in the HandleLeaderRedirection functions.
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3545522300
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -41,6 +41,7 @@ func (c *IggyTcpClient) LoginUser(ctx context.Context, username string, password
c.logger.Info("Iggy client has signed in successfully.", slog.String("client_address", c.clientAddress))
identity := binaryserialization.DeserializeLogInResponse(buffer)
+ c.events.publish(iggcon.DiagnosticEventSignedIn)
Review Comment:
@explise Your concern is correst, I didn't thought it throughly. Technically, because the redirect needs to call `GetClusterMetadata`, a session is required. It's true that we will log in every time before we redirect, so keeping the current approach might be the best choice(publish the login event multiple times, since it's true that we logined).
So it may be better to keep this `publish_event...`, ignore my suggestion to move this to the end of SignIn function.
But I still believe it might be better to add the redirect event. I checked the code of IggyProducer and IggyConsumer, if the redirect event is provided, it would help it to filter the noise during redirection.
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #3621:
URL: https://github.com/apache/iggy/pull/3621#issuecomment-4897700964
## [Codecov](https://app.codecov.io/gh/apache/iggy/pull/3621?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `53.96825%` with `29 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 73.18%. Comparing base ([`6b908ca`](https://app.codecov.io/gh/apache/iggy/commit/6b908ca1efac1bd97736894887c48b2fcfb18aa9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5602867`](https://app.codecov.io/gh/apache/iggy/commit/56028675a9fd20e284a1907f96eaf4557e801e22?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/iggy/pull/3621?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [foreign/go/contracts/diagnostic\_event.go](https://app.codecov.io/gh/apache/iggy/pull/3621?src=pr&el=tree&filepath=foreign%2Fgo%2Fcontracts%2Fdiagnostic_event.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Zm9yZWlnbi9nby9jb250cmFjdHMvZGlhZ25vc3RpY19ldmVudC5nbw==) | 0.00% | [14 Missing :warning: ](https://app.codecov.io/gh/apache/iggy/pull/3621?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [foreign/go/client/tcp/tcp\_core.go](https://app.codecov.io/gh/apache/iggy/pull/3621?src=pr&el=tree&filepath=foreign%2Fgo%2Fclient%2Ftcp%2Ftcp_core.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Zm9yZWlnbi9nby9jbGllbnQvdGNwL3RjcF9jb3JlLmdv) | 12.50% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/iggy/pull/3621?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [foreign/go/client/tcp/tcp\_session\_management.go](https://app.codecov.io/gh/apache/iggy/pull/3621?src=pr&el=tree&filepath=foreign%2Fgo%2Fclient%2Ftcp%2Ftcp_session_management.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Zm9yZWlnbi9nby9jbGllbnQvdGNwL3RjcF9zZXNzaW9uX21hbmFnZW1lbnQuZ28=) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/iggy/pull/3621?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [foreign/go/client/tcp/tcp\_events.go](https://app.codecov.io/gh/apache/iggy/pull/3621?src=pr&el=tree&filepath=foreign%2Fgo%2Fclient%2Ftcp%2Ftcp_events.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Zm9yZWlnbi9nby9jbGllbnQvdGNwL3RjcF9ldmVudHMuZ28=) | 94.28% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/iggy/pull/3621?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #3621 +/- ##
============================================
- Coverage 74.11% 73.18% -0.93%
Complexity 937 937
============================================
Files 1262 1264 +2
Lines 132757 132818 +61
Branches 108645 108645
============================================
- Hits 98395 97206 -1189
- Misses 31273 32613 +1340
+ Partials 3089 2999 -90
```
| [Components](https://app.codecov.io/gh/apache/iggy/pull/3621/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [Rust Core](https://app.codecov.io/gh/apache/iggy/pull/3621/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `74.51% <ø> (ø)` | |
| [Java SDK](https://app.codecov.io/gh/apache/iggy/pull/3621/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.44% <ø> (ø)` | |
| [C# SDK](https://app.codecov.io/gh/apache/iggy/pull/3621/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.04% <ø> (ø)` | |
| [Python SDK](https://app.codecov.io/gh/apache/iggy/pull/3621/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `91.65% <ø> (ø)` | |
| [PHP SDK](https://app.codecov.io/gh/apache/iggy/pull/3621/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `84.29% <ø> (ø)` | |
| [Node SDK](https://app.codecov.io/gh/apache/iggy/pull/3621/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `91.35% <ø> (ø)` | |
| [Go SDK](https://app.codecov.io/gh/apache/iggy/pull/3621/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `17.00% <53.96%> (-25.29%)` | :arrow_down: |
| [Files with missing lines](https://app.codecov.io/gh/apache/iggy/pull/3621?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [foreign/go/client/tcp/tcp\_events.go](https://app.codecov.io/gh/apache/iggy/pull/3621?src=pr&el=tree&filepath=foreign%2Fgo%2Fclient%2Ftcp%2Ftcp_events.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Zm9yZWlnbi9nby9jbGllbnQvdGNwL3RjcF9ldmVudHMuZ28=) | `94.28% <94.28%> (ø)` | |
| [foreign/go/client/tcp/tcp\_session\_management.go](https://app.codecov.io/gh/apache/iggy/pull/3621?src=pr&el=tree&filepath=foreign%2Fgo%2Fclient%2Ftcp%2Ftcp_session_management.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Zm9yZWlnbi9nby9jbGllbnQvdGNwL3RjcF9zZXNzaW9uX21hbmFnZW1lbnQuZ28=) | `0.00% <0.00%> (-57.15%)` | :arrow_down: |
| [foreign/go/client/tcp/tcp\_core.go](https://app.codecov.io/gh/apache/iggy/pull/3621?src=pr&el=tree&filepath=foreign%2Fgo%2Fclient%2Ftcp%2Ftcp_core.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Zm9yZWlnbi9nby9jbGllbnQvdGNwL3RjcF9jb3JlLmdv) | `38.19% <12.50%> (-12.86%)` | :arrow_down: |
| [foreign/go/contracts/diagnostic\_event.go](https://app.codecov.io/gh/apache/iggy/pull/3621?src=pr&el=tree&filepath=foreign%2Fgo%2Fcontracts%2Fdiagnostic_event.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Zm9yZWlnbi9nby9jb250cmFjdHMvZGlhZ25vc3RpY19ldmVudC5nbw==) | `0.00% <0.00%> (ø)` | |
... and [40 files with indirect coverage changes](https://app.codecov.io/gh/apache/iggy/pull/3621/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on PR #3621:
URL: https://github.com/apache/iggy/pull/3621#issuecomment-4917512322
btw you don't have to keep amend and force push. The pr would be squashed before merge anyway.
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3545522300
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -41,6 +41,7 @@ func (c *IggyTcpClient) LoginUser(ctx context.Context, username string, password
c.logger.Info("Iggy client has signed in successfully.", slog.String("client_address", c.clientAddress))
identity := binaryserialization.DeserializeLogInResponse(buffer)
+ c.events.publish(iggcon.DiagnosticEventSignedIn)
Review Comment:
@explise Your concern is correst, I didn't thought it throughly. Technically, because the redirect needs to call `GetClusterMetadata`, a session is required. It's true that we will log in every time before we redirect, so keeping the current approach might be the best choice(publish the login event multiple times, since it's true that we logined). While they are noise, they tell the subscriber these events actually happened.
So it may be better to keep this `publish_event...`, ignore my suggestion to move this to the end of SignIn function.
But I still believe it might be better to add the redirect event. I checked the code of IggyProducer and IggyConsumer, if the redirect event is provided, it would help it to filter the noise during redirection. For example, when received the redirect event, ignore the following sign in ,connect, disconnect. @hubcio would that help?
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "explise (via GitHub)" <gi...@apache.org>.
explise commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3542766946
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -41,6 +41,7 @@ func (c *IggyTcpClient) LoginUser(ctx context.Context, username string, password
c.logger.Info("Iggy client has signed in successfully.", slog.String("client_address", c.clientAddress))
identity := binaryserialization.DeserializeLogInResponse(buffer)
+ c.events.publish(iggcon.DiagnosticEventSignedIn)
Review Comment:
rust doesn't have anything similiar and adding redirection breaks the parity here @hubcio what do u think about it here
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3575949877
##########
foreign/go/client/tcp/tcp_core.go:
##########
Review Comment:
```go
var err error
if c.conn != nil {
err = c.conn.Close()
}
```
This part on hold, I need to consider about 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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3542916693
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -41,6 +41,7 @@ func (c *IggyTcpClient) LoginUser(ctx context.Context, username string, password
c.logger.Info("Iggy client has signed in successfully.", slog.String("client_address", c.clientAddress))
identity := binaryserialization.DeserializeLogInResponse(buffer)
+ c.events.publish(iggcon.DiagnosticEventSignedIn)
Review Comment:
unfortunately my golang knowledge is second to none, sorry. @chengxilo any thoughts on above question?
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "explise (via GitHub)" <gi...@apache.org>.
explise commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3545725562
##########
foreign/go/tests/diagnostic_events_test.go:
##########
@@ -0,0 +1,273 @@
+// 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 tests_test
+
+import (
+ "context"
+ "fmt"
+ "os"
+ "testing"
+ "time"
+
+ "github.com/apache/iggy/foreign/go/client"
+ "github.com/apache/iggy/foreign/go/client/tcp"
+ iggcon "github.com/apache/iggy/foreign/go/contracts"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+ "github.com/testcontainers/testcontainers-go"
+ "github.com/testcontainers/testcontainers-go/wait"
+)
+
+// setupPlainContainer starts a non-TLS Iggy server container.
+func setupPlainContainer(t *testing.T) (testcontainers.Container, string) {
+ ctx := context.Background()
+
+ dockerImage := os.Getenv("IGGY_SERVER_DOCKER_IMAGE")
+ if dockerImage == "" {
+ dockerImage = "apache/iggy:edge"
+ }
+
+ req := testcontainers.ContainerRequest{
+ Image: dockerImage,
+ ExposedPorts: []string{"8090/tcp"},
+ Env: map[string]string{
+ "IGGY_ROOT_USERNAME": "iggy",
+ "IGGY_ROOT_PASSWORD": "iggy",
+ "IGGY_TCP_ADDRESS": "0.0.0.0:8090",
+ },
+ WaitingFor: wait.ForAll(
+ wait.ForLog("Iggy server is running"),
+ wait.ForListeningPort("8090/tcp"),
+ ),
+ Privileged: true,
+ AutoRemove: true,
+ }
+
+ container, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{
+ ContainerRequest: req,
+ Started: true,
+ })
+ require.NoError(t, err, "Failed to start Iggy container")
+
+ host, err := container.Host(ctx)
+ require.NoError(t, err)
+ port, err := container.MappedPort(ctx, "8090")
+ require.NoError(t, err)
+ addr := fmt.Sprintf("%s:%s", host, port.Port())
+
+ waitForServer(t, addr, 60*time.Second)
+
+ return container, addr
+}
+
+// recvEvent reads one event from the channel with a timeout. Failing the
+// test on timeout keeps a flaky CI from hanging the whole suite.
+func recvEvent(t *testing.T, ch <-chan iggcon.DiagnosticEvent, timeout time.Duration) (iggcon.DiagnosticEvent, bool) {
+ t.Helper()
+ select {
+ case ev, ok := <-ch:
+ return ev, ok
+ case <-time.After(timeout):
+ t.Fatalf("timed out after %v waiting for diagnostic event", timeout)
+ return 0, false
+ }
+}
+
+// drainEvents collects events until the channel is closed or the timeout
+// expires. Used to assert the full sequence emitted during a scenario.
+func drainEvents(t *testing.T, ch <-chan iggcon.DiagnosticEvent, timeout time.Duration) []iggcon.DiagnosticEvent {
+ t.Helper()
+ deadline := time.After(timeout)
+ var events []iggcon.DiagnosticEvent
+ for {
+ select {
+ case ev, ok := <-ch:
+ if !ok {
+ return events
+ }
+ events = append(events, ev)
+ case <-deadline:
+ t.Fatalf("timed out after %v draining diagnostic events; got %v", timeout, events)
+ return events
+ }
+ }
+}
+
+func newPlainClient(t *testing.T, addr string) iggcon.Client {
+ t.Helper()
+ cli, err := client.NewIggyClient(
+ client.WithTcp(
+ tcp.WithServerAddress(addr),
+ ),
+ )
+ require.NoError(t, err, "Failed to create Iggy client")
+ return cli
+}
+
+// TestDiagnosticEvents_LoginEmitsSignedIn verifies that LoginUser emits
+// a SignedIn event after a successful authentication.
+func TestDiagnosticEvents_LoginEmitsSignedIn(t *testing.T) {
+ container, addr := setupPlainContainer(t)
+ defer func() {
+ if err := container.Terminate(context.Background()); err != nil {
+ t.Logf("Failed to terminate container: %v", err)
+ }
+ }()
Review Comment:
fixed
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3545522300
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -41,6 +41,7 @@ func (c *IggyTcpClient) LoginUser(ctx context.Context, username string, password
c.logger.Info("Iggy client has signed in successfully.", slog.String("client_address", c.clientAddress))
identity := binaryserialization.DeserializeLogInResponse(buffer)
+ c.events.publish(iggcon.DiagnosticEventSignedIn)
Review Comment:
@explise Your concern is correct, I didn't thought it throughly. Technically, because the redirect needs to call `GetClusterMetadata`, a session is required. It's true that we will log in every time before we redirect, so keeping the current approach might be the best choice(publish the login event multiple times, since it's true that we logined). While they are noise, they tell the subscriber these events actually happened.
So it may be better to keep this `publish_event...`, ignore my suggestion to move this to the end of SignIn function.
But I still believe it might be better to add the redirect event. I checked the code of IggyProducer and IggyConsumer, if the redirect event is provided, it would help it to filter the noise during redirection. For example, when received the redirect event, ignore the following `disconnect`, `connect`,`disconnect`. @hubcio would that help?
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3539160336
##########
foreign/go/client/tcp/tcp_core.go:
##########
Review Comment:
```suggestion
c.state = iggcon.StateDisconnected
c.events.publish(iggcon.DiagnosticEventDisconnected)
```
As `invalidateConnLocked` also set the state to `StateDisconnected`, maybe we should also publish the event here :)
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
Review Comment:
```suggestion
c.events.publish(iggcon.DiagnosticEventSignedIn)
return identity, nil
```
After all these changes, when redirection happens, the event would be:
```
Connected
Redirected
Disconnected
Connected
SignedIn
```
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -41,6 +41,7 @@ func (c *IggyTcpClient) LoginUser(ctx context.Context, username string, password
c.logger.Info("Iggy client has signed in successfully.", slog.String("client_address", c.clientAddress))
identity := binaryserialization.DeserializeLogInResponse(buffer)
+ c.events.publish(iggcon.DiagnosticEventSignedIn)
shouldRedirect, err := c.HandleLeaderRedirection(ctx)
Review Comment:
In `HandleLeaderRedirection`, publish the `DiagnosticEventRedirected`. Because this file is not touched, I chose to comment here
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -41,6 +41,7 @@ func (c *IggyTcpClient) LoginUser(ctx context.Context, username string, password
c.logger.Info("Iggy client has signed in successfully.", slog.String("client_address", c.clientAddress))
identity := binaryserialization.DeserializeLogInResponse(buffer)
+ c.events.publish(iggcon.DiagnosticEventSignedIn)
Review Comment:
```suggestion
```
Publishing the SignedIn event here means multiple SignedIn events will be emitted during a redirection. Like this:
```
Connected
SignedIn
Disconnected
Connected
SignedIn
```
While this is expected behavior and not a bug, we could make the flow more elegant and informative. I suggest publishing a DiagnosticEventRedirected during redirection to clarify that a redirect is occurring rather than a glitch. See my following comment for the suggested implementation : )
##########
foreign/go/contracts/client.go:
##########
@@ -29,6 +29,10 @@ type Client interface {
// GetConnectionInfo returns the current connection information including protocol and server address
GetConnectionInfo() *ConnectionInfo
+ // SubscribeEvents returns an independent channel of client lifecycle events.
+ // The channel is closed on shutdown, after the final DiagnosticEventShutdown.
+ SubscribeEvents() <-chan DiagnosticEvent
Review Comment:
We should also support unsubscribe. Let this function return a `func()` that can unsubscribe it. So we can call it like:
```go
events, cancel : = SubscribeEvents()
defer cancel ()
// .... use events
```
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
Review Comment:
same as above.
##########
foreign/go/tests/diagnostic_events_test.go:
##########
@@ -0,0 +1,273 @@
+// 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 tests_test
+
+import (
+ "context"
+ "fmt"
+ "os"
+ "testing"
+ "time"
+
+ "github.com/apache/iggy/foreign/go/client"
+ "github.com/apache/iggy/foreign/go/client/tcp"
+ iggcon "github.com/apache/iggy/foreign/go/contracts"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+ "github.com/testcontainers/testcontainers-go"
+ "github.com/testcontainers/testcontainers-go/wait"
+)
+
+// setupPlainContainer starts a non-TLS Iggy server container.
+func setupPlainContainer(t *testing.T) (testcontainers.Container, string) {
+ ctx := context.Background()
+
+ dockerImage := os.Getenv("IGGY_SERVER_DOCKER_IMAGE")
+ if dockerImage == "" {
+ dockerImage = "apache/iggy:edge"
+ }
+
+ req := testcontainers.ContainerRequest{
+ Image: dockerImage,
+ ExposedPorts: []string{"8090/tcp"},
+ Env: map[string]string{
+ "IGGY_ROOT_USERNAME": "iggy",
+ "IGGY_ROOT_PASSWORD": "iggy",
+ "IGGY_TCP_ADDRESS": "0.0.0.0:8090",
+ },
+ WaitingFor: wait.ForAll(
+ wait.ForLog("Iggy server is running"),
+ wait.ForListeningPort("8090/tcp"),
+ ),
+ Privileged: true,
+ AutoRemove: true,
+ }
+
+ container, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{
+ ContainerRequest: req,
+ Started: true,
+ })
+ require.NoError(t, err, "Failed to start Iggy container")
+
+ host, err := container.Host(ctx)
+ require.NoError(t, err)
+ port, err := container.MappedPort(ctx, "8090")
+ require.NoError(t, err)
+ addr := fmt.Sprintf("%s:%s", host, port.Port())
+
+ waitForServer(t, addr, 60*time.Second)
+
+ return container, addr
+}
+
+// recvEvent reads one event from the channel with a timeout. Failing the
+// test on timeout keeps a flaky CI from hanging the whole suite.
+func recvEvent(t *testing.T, ch <-chan iggcon.DiagnosticEvent, timeout time.Duration) (iggcon.DiagnosticEvent, bool) {
+ t.Helper()
+ select {
+ case ev, ok := <-ch:
+ return ev, ok
+ case <-time.After(timeout):
+ t.Fatalf("timed out after %v waiting for diagnostic event", timeout)
+ return 0, false
+ }
+}
+
+// drainEvents collects events until the channel is closed or the timeout
+// expires. Used to assert the full sequence emitted during a scenario.
+func drainEvents(t *testing.T, ch <-chan iggcon.DiagnosticEvent, timeout time.Duration) []iggcon.DiagnosticEvent {
+ t.Helper()
+ deadline := time.After(timeout)
+ var events []iggcon.DiagnosticEvent
+ for {
+ select {
+ case ev, ok := <-ch:
+ if !ok {
+ return events
+ }
+ events = append(events, ev)
+ case <-deadline:
+ t.Fatalf("timed out after %v draining diagnostic events; got %v", timeout, events)
+ return events
+ }
+ }
+}
+
+func newPlainClient(t *testing.T, addr string) iggcon.Client {
+ t.Helper()
+ cli, err := client.NewIggyClient(
+ client.WithTcp(
+ tcp.WithServerAddress(addr),
+ ),
+ )
+ require.NoError(t, err, "Failed to create Iggy client")
+ return cli
+}
+
+// TestDiagnosticEvents_LoginEmitsSignedIn verifies that LoginUser emits
+// a SignedIn event after a successful authentication.
+func TestDiagnosticEvents_LoginEmitsSignedIn(t *testing.T) {
+ container, addr := setupPlainContainer(t)
+ defer func() {
+ if err := container.Terminate(context.Background()); err != nil {
+ t.Logf("Failed to terminate container: %v", err)
+ }
+ }()
Review Comment:
nit: `defer`s can be replaced by adding `t.Cleanup()` in setupPlainContainer.
And actually, I think it's a little bit over kill to create container over and over again, it should be better to just use `TestMain`. We create the container, run the tests, and terminate 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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3621:
URL: https://github.com/apache/iggy/pull/3621#issuecomment-5124921569
This pull request was automatically closed because it has been inactive for 14 days. Feel free to reopen it if you'd like to continue working on 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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #3621: feat(go): add diagnostic event subscriptions to TCP client
URL: https://github.com/apache/iggy/pull/3621
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3575236369
##########
foreign/go/client/tcp/tcp_core.go:
##########
Review Comment:
Replace these code with:
```go
var err error
if c.conn != nil {
err = c.conn.Close()
}
c.conn = nil
```
If we return the error directly, we won't be able to close the eventBroadcaster.
I also add `c.conn = nil`, it's not a problem introduced by this PR, but its an historical issue that we never set the connection to nil after closeing it while we already have the nil guard.
Similarly, I add suggestion to set `c.conn = nil` under `invalidateConnLocked` and `disconnect` too
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3545522300
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -41,6 +41,7 @@ func (c *IggyTcpClient) LoginUser(ctx context.Context, username string, password
c.logger.Info("Iggy client has signed in successfully.", slog.String("client_address", c.clientAddress))
identity := binaryserialization.DeserializeLogInResponse(buffer)
+ c.events.publish(iggcon.DiagnosticEventSignedIn)
Review Comment:
@explise Your concern is correst, I didn't thought it throughly. Technically, because the redirect needs to call `GetClusterMetadata`, a session is required. It's true that we will log in every time before we redirect, so keeping the current approach might be the best choice(publish the login event multiple times, since it's true that we logined).
So it may be better to keep this `publish_event...`, ignore my suggestion to move this to the end of SignIn function.
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3542916693
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -41,6 +41,7 @@ func (c *IggyTcpClient) LoginUser(ctx context.Context, username string, password
c.logger.Info("Iggy client has signed in successfully.", slog.String("client_address", c.clientAddress))
identity := binaryserialization.DeserializeLogInResponse(buffer)
+ c.events.publish(iggcon.DiagnosticEventSignedIn)
Review Comment:
unfortunately my golang knowledge is second to none, sorry. @chengxilo any thoughts on above question?
from Rust perspective, feel free to break parity. i dont see any reason to stick to current approach, if you can figure out something better maybe we even can implement it in Rust SDK also.
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3621:
URL: https://github.com/apache/iggy/pull/3621#issuecomment-5040539078
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!
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "chengxilo (via GitHub)" <gi...@apache.org>.
chengxilo commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3574924254
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -117,6 +122,8 @@ func (c *IggyTcpClient) HandleLeaderRedirection(ctx context.Context) (bool, erro
}
c.mtx.Unlock()
+ c.events.publish(iggcon.DiagnosticEventRedirected)
Review Comment:
It may be a better choice to move this to the last part of this function, i.e. after the `disconnect()` call.
```go
c.mtx.Lock()
...
c.events.publish(iggcon.DiagnosticEventRedirected)
c.mtx.Unlock()
```
When we receive false, we will not handle the redirect but return an error.
##########
foreign/go/client/tcp/tcp_events.go:
##########
@@ -0,0 +1,116 @@
+// 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 tcp
+
+import (
+ "sync"
+
+ iggcon "github.com/apache/iggy/foreign/go/contracts"
+)
+
+// subscriberBufferSize bounds each subscriber's channel. A subscriber that
+// fails to drain within this many events will start dropping the oldest
+// events; the publisher is never blocked.
+const subscriberBufferSize = 1000
+
+// eventBroadcaster fans out diagnostic events to any number of subscribers.
+// Each call to subscribe returns an independent channel; publish delivers
+// the event to every live subscriber non-blockingly. close releases all
+// subscriber channels and is idempotent.
+type eventBroadcaster struct {
+ mu sync.Mutex
+ subscribers []chan iggcon.DiagnosticEvent
+ closed bool
+}
+
+func newEventBroadcaster() *eventBroadcaster {
+ return &eventBroadcaster{}
+}
+
+// subscribe returns a new subscriber channel and an unsubscribe function that
+// is idempotent and safe to call after the broadcaster is closed.
+func (b *eventBroadcaster) subscribe() (<-chan iggcon.DiagnosticEvent, func()) {
+ b.mu.Lock()
+ defer b.mu.Unlock()
+
+ ch := make(chan iggcon.DiagnosticEvent, subscriberBufferSize)
+ if b.closed {
+ close(ch)
+ return ch, func() {}
+ }
+ b.subscribers = append(b.subscribers, ch)
+
+ var once sync.Once
+ unsubscribe := func() {
+ once.Do(func() {
+ b.mu.Lock()
+ defer b.mu.Unlock()
+ if b.closed {
+ return
+ }
+ for i, sub := range b.subscribers {
+ if sub == ch {
+ b.subscribers = append(b.subscribers[:i], b.subscribers[i+1:]...)
+ close(ch)
+ return
+ }
+ }
+ })
+ }
+ return ch, unsubscribe
+}
+
+func (b *eventBroadcaster) publish(event iggcon.DiagnosticEvent) {
+ b.mu.Lock()
+ defer b.mu.Unlock()
+
+ if b.closed {
+ return
+ }
+ for _, ch := range b.subscribers {
+ select {
+ case ch <- event:
+ default:
+ // Subscriber is not draining fast enough. Drop the oldest event
+ // to make room for the newest, preferring recency. Keeps the
+ // publisher non-blocking even with a slow subscriber.
+ select {
+ case <-ch:
+ default:
+ }
+ select {
+ case ch <- event:
+ default:
+ }
Review Comment:
```suggestion
ch <- event
```
This should be enough.
There are two possible situation for the previous select {}.
First one, `case <- ch:`, it means there is one free space in `ch`, it won't be stucked.
Second one, `default`, it means `ch` is empty, it won't be stucked neither.
So `ch <- event` without the `select { default: }` is already enough. : )
##########
foreign/go/client/tcp/tcp_events.go:
##########
@@ -0,0 +1,116 @@
+// 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 tcp
+
+import (
+ "sync"
+
+ iggcon "github.com/apache/iggy/foreign/go/contracts"
+)
+
+// subscriberBufferSize bounds each subscriber's channel. A subscriber that
+// fails to drain within this many events will start dropping the oldest
+// events; the publisher is never blocked.
+const subscriberBufferSize = 1000
+
+// eventBroadcaster fans out diagnostic events to any number of subscribers.
+// Each call to subscribe returns an independent channel; publish delivers
+// the event to every live subscriber non-blockingly. close releases all
+// subscriber channels and is idempotent.
+type eventBroadcaster struct {
+ mu sync.Mutex
+ subscribers []chan iggcon.DiagnosticEvent
+ closed bool
+}
+
+func newEventBroadcaster() *eventBroadcaster {
+ return &eventBroadcaster{}
+}
+
+// subscribe returns a new subscriber channel and an unsubscribe function that
+// is idempotent and safe to call after the broadcaster is closed.
+func (b *eventBroadcaster) subscribe() (<-chan iggcon.DiagnosticEvent, func()) {
+ b.mu.Lock()
+ defer b.mu.Unlock()
+
+ ch := make(chan iggcon.DiagnosticEvent, subscriberBufferSize)
+ if b.closed {
+ close(ch)
+ return ch, func() {}
+ }
+ b.subscribers = append(b.subscribers, ch)
+
+ var once sync.Once
+ unsubscribe := func() {
+ once.Do(func() {
+ b.mu.Lock()
+ defer b.mu.Unlock()
+ if b.closed {
+ return
+ }
+ for i, sub := range b.subscribers {
+ if sub == ch {
+ b.subscribers = append(b.subscribers[:i], b.subscribers[i+1:]...)
Review Comment:
nit: `slices.Delete` is recommended.
##########
foreign/go/client/tcp/tcp_events.go:
##########
@@ -0,0 +1,116 @@
+// 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 tcp
+
+import (
+ "sync"
+
+ iggcon "github.com/apache/iggy/foreign/go/contracts"
+)
+
+// subscriberBufferSize bounds each subscriber's channel. A subscriber that
+// fails to drain within this many events will start dropping the oldest
+// events; the publisher is never blocked.
+const subscriberBufferSize = 1000
+
+// eventBroadcaster fans out diagnostic events to any number of subscribers.
+// Each call to subscribe returns an independent channel; publish delivers
+// the event to every live subscriber non-blockingly. close releases all
+// subscriber channels and is idempotent.
+type eventBroadcaster struct {
+ mu sync.Mutex
+ subscribers []chan iggcon.DiagnosticEvent
+ closed bool
+}
+
+func newEventBroadcaster() *eventBroadcaster {
+ return &eventBroadcaster{}
+}
+
+// subscribe returns a new subscriber channel and an unsubscribe function that
+// is idempotent and safe to call after the broadcaster is closed.
+func (b *eventBroadcaster) subscribe() (<-chan iggcon.DiagnosticEvent, func()) {
+ b.mu.Lock()
+ defer b.mu.Unlock()
+
+ ch := make(chan iggcon.DiagnosticEvent, subscriberBufferSize)
+ if b.closed {
+ close(ch)
+ return ch, func() {}
+ }
+ b.subscribers = append(b.subscribers, ch)
+
+ var once sync.Once
+ unsubscribe := func() {
+ once.Do(func() {
+ b.mu.Lock()
+ defer b.mu.Unlock()
+ if b.closed {
+ return
+ }
Review Comment:
```suggestion
```
nit: when eventBroadcaster is closed, the slice would be nil, it's fine to remove this guard in my opinion.
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -431,12 +433,16 @@ func (c *IggyTcpClient) sendLocked(wirePayload []byte) ([]byte, error) {
return buffer, nil
}
-// invalidateConnLocked closes the connection and marks it as disconnected
+// invalidateConnLocked closes the connection and marks it as disconnected,
+// publishing a Disconnected event on the transition.
func (c *IggyTcpClient) invalidateConnLocked() {
if c.conn != nil {
_ = c.conn.Close()
Review Comment:
```suggestion
_ = c.conn.Close()
c.conn = nil
```
See my coment in `shutdown` for detail.
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -558,6 +564,7 @@ func (c *IggyTcpClient) Connect(ctx context.Context) error {
c.connectedAt = time.Now()
c.logger.Info("Iggy client has connected to the Iggy server", slog.String("client_address", c.clientAddress), slog.String("server_address", c.currentServerAddress))
c.mtx.Unlock()
+ c.events.publish(iggcon.DiagnosticEventConnected)
Review Comment:
maybe we can do this before `c.mtx.Unlock()`.
##########
foreign/go/tests/diagnostic_events_test.go:
##########
@@ -0,0 +1,268 @@
+// 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 tests_test
+
+import (
+ "context"
+ "fmt"
+ "os"
+ "testing"
+ "time"
+
+ "github.com/apache/iggy/foreign/go/client"
+ "github.com/apache/iggy/foreign/go/client/tcp"
+ iggcon "github.com/apache/iggy/foreign/go/contracts"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+ "github.com/testcontainers/testcontainers-go"
+ "github.com/testcontainers/testcontainers-go/wait"
+)
+
+// sharedServerAddr is the address of the Iggy server started once for the
+// whole package by TestMain and reused by every diagnostic-event test.
+var sharedServerAddr string
+
+// TestMain starts one Iggy server container for the whole package, runs the
+// tests against it, and tears it down.
+func TestMain(m *testing.M) {
+ ctx := context.Background()
+
+ container, addr, err := startPlainContainer(ctx)
+ if err != nil {
+ fmt.Fprintf(os.Stderr, "diagnostic_events_test: failed to start Iggy container: %v\n", err)
+ os.Exit(1)
+ }
Review Comment:
If `err != nil`, it won't terminate the container, as it `os.Exit(1)` directly, better try to terminate it or something.
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -624,7 +631,7 @@ func (c *IggyTcpClient) disconnect() error {
}
Review Comment:
Save the error and return at the end of this function, also, set c.conn to nil.
```go
var err error
if c.conn != nil {
err = c.conn.Close()
}
c.conn = nil
```
See my coment in `shutdown` for detail.
##########
foreign/go/client/tcp/tcp_events.go:
##########
@@ -0,0 +1,116 @@
+// 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 tcp
+
+import (
+ "sync"
+
+ iggcon "github.com/apache/iggy/foreign/go/contracts"
+)
+
+// subscriberBufferSize bounds each subscriber's channel. A subscriber that
+// fails to drain within this many events will start dropping the oldest
+// events; the publisher is never blocked.
+const subscriberBufferSize = 1000
+
+// eventBroadcaster fans out diagnostic events to any number of subscribers.
+// Each call to subscribe returns an independent channel; publish delivers
+// the event to every live subscriber non-blockingly. close releases all
+// subscriber channels and is idempotent.
+type eventBroadcaster struct {
+ mu sync.Mutex
+ subscribers []chan iggcon.DiagnosticEvent
+ closed bool
+}
+
+func newEventBroadcaster() *eventBroadcaster {
+ return &eventBroadcaster{}
+}
+
+// subscribe returns a new subscriber channel and an unsubscribe function that
+// is idempotent and safe to call after the broadcaster is closed.
+func (b *eventBroadcaster) subscribe() (<-chan iggcon.DiagnosticEvent, func()) {
+ b.mu.Lock()
+ defer b.mu.Unlock()
+
+ ch := make(chan iggcon.DiagnosticEvent, subscriberBufferSize)
+ if b.closed {
+ close(ch)
+ return ch, func() {}
+ }
+ b.subscribers = append(b.subscribers, ch)
+
+ var once sync.Once
+ unsubscribe := func() {
+ once.Do(func() {
Review Comment:
nit: `once.Do` can also be waived, we will iterate the slice and find the channel we want to remove, if it is already removed we will do nothing.
I think iterate on a short slice will cost less resource than `once.Do`. It's good and safe to have it, but I think it's fine to waive it.
##########
foreign/go/client/tcp/tcp_core.go:
##########
Review Comment:
```go
var err error
if c.conn != nil {
err = c.conn.Close()
}
c.conn = nil
```
If we return the error directly, we won't be able to close the eventBroadcaster.
I also add `c.conn = nil`, it's not a problem introduced by this PR, but its an historical issue that we never set the connection to nil after closeing it while we already have the nil guard.
Similarly, I add suggestion to set `c.conn = nil` under `invalidateConnLocked` and `disconnect` too
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -60,6 +60,7 @@ type IggyTcpClient struct {
currentServerAddress string
connectedAt time.Time
state iggcon.State
+ events *eventBroadcaster
Review Comment:
nit: I think we don't have to use pointer for it. eventBroadcaster should be enough. And when initialize it we can use `events: eventBroadcaster{}`
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -548,7 +554,7 @@ func (c *IggyTcpClient) Connect(ctx context.Context) error {
if !c.config.reconnection.enabled {
c.logger.Warn("Automatic reconnection is disabled.")
}
- // TODO publish event disconnected
+ c.events.publish(iggcon.DiagnosticEventDisconnected)
Review Comment:
maybe we can do this before c.mtx.Unlock()
--
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] feat(go): add diagnostic event subscriptions to TCP client [iggy]
Posted by "explise (via GitHub)" <gi...@apache.org>.
explise commented on code in PR #3621:
URL: https://github.com/apache/iggy/pull/3621#discussion_r3573238977
##########
foreign/go/contracts/client.go:
##########
@@ -29,6 +29,10 @@ type Client interface {
// GetConnectionInfo returns the current connection information including protocol and server address
GetConnectionInfo() *ConnectionInfo
+ // SubscribeEvents returns an independent channel of client lifecycle events.
+ // The channel is closed on shutdown, after the final DiagnosticEventShutdown.
+ SubscribeEvents() <-chan DiagnosticEvent
Review Comment:
fixed
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
Review Comment:
fixed
--
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