Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix nats reconnects #8880

Merged
merged 2 commits into from
Apr 18, 2024
Merged

Fix nats reconnects #8880

merged 2 commits into from
Apr 18, 2024

Conversation

kobergj
Copy link
Collaborator

@kobergj kobergj commented Apr 17, 2024

Repairs reconnects on the natsjs kv registry

New behaviour:

  • when nats gets disconnected -> no change
  • when nats gets reconnected -> registry reconnects correctly
  • when nats connection is closed -> os.Exit as there is no way to recover without initializing everything

Fixes #8783

@kobergj kobergj marked this pull request as ready for review April 18, 2024 09:29
Copy link
Contributor

@rhafer rhafer left a comment

Choose a reason for hiding this comment

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

Please change the changelog message wording. Other than that I am fine with it.

changelog/unreleased/nats-reconnects.md Outdated Show resolved Hide resolved
natsOptions.ReconnectedCB = func(_ *nats.Conn) {
if err := n.Init(); err != nil {
fmt.Println("cannot reconnect to nats")
os.Exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

💥 OUCH

I guess I understand why you're doing that. And I don't have a better solution at hand. But I still feel the need to say: This is ugly 😄

Would be nice if we could find a way for a more graceful error handling here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I 100% agree. ❤️ I'll fix it as soon as we have a proper solution.

@kobergj kobergj requested a review from rhafer April 18, 2024 11:07
Copy link
Contributor

@rhafer rhafer left a comment

Choose a reason for hiding this comment

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

LGTM, but now Sonarcloud doesn't like you anymore ...

Co-authored-by: Ralf Haferkamp <[email protected]>
Signed-off-by: jkoberg <[email protected]>
Copy link

@kobergj
Copy link
Collaborator Author

kobergj commented Apr 18, 2024

No code changes - just force pushed to restart sonarcloud.

@kobergj kobergj merged commit 6c3e730 into owncloud:master Apr 18, 2024
4 checks passed
@kobergj kobergj deleted the NatsReconnects branch April 18, 2024 13:01
ownclouders pushed a commit that referenced this pull request Apr 18, 2024
@wkloucek
Copy link
Contributor

wkloucek commented May 13, 2024

@micbar @kobergj is this suitable for a backport to oCIS 5.0.x ? I have support cases with this.

@micbar
Copy link
Contributor

micbar commented May 13, 2024

That depends. IMHO we need to discusss if this is a patch according to semver.

@kobergj @butonic @rhafer what do you think?

@rhafer
Copy link
Contributor

rhafer commented May 13, 2024

@micbar Why do think it could be problematic regarding semver? It's a bugfix. It doesn't need new config. Neither does it break existing setups (does it?).

@micbar
Copy link
Contributor

micbar commented May 13, 2024

then let us backport.

@ScharfViktor ScharfViktor mentioned this pull request May 14, 2024
19 tasks
@micbar micbar mentioned this pull request Jun 19, 2024
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

healthcheck not failing even when service registration is not successful
4 participants