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

Support to use a custom database number #189

Merged
merged 5 commits into from
Sep 12, 2023

Conversation

ardecvz
Copy link
Contributor

@ardecvz ardecvz commented Sep 11, 2023

What is the purpose of this pull request?

Support to use a custom database number.

What changes did you make? (overview)

Parse --redis_url (ANYCABLE_REDIS_URL or REDIS_URL) using the rueidis standard utility.

Is there anything you'd like reviewers to focus on?

WARNING! This could be a potentially breaking change. Prior to this update, AnyCable Go did not support database numbers but allowed them in the REDIS_URL. As a result, if a user had set a URL with a database number, all data would still be written to the default database (0). After upgrading, this will break the existing user setup by starting to write data to the specified in the URL database, potentially leaving remnants in the old one.

@palkan, it's critical for you to assess how important the data in the storage is and decide on the appropriate path for data migration (whether that be discarding the old data, issuing a warning, performing automatic migration, etc.).

Checklist

  • I've added tests for this change
  • I've added a Changelog entry
  • I've updated documentation

Fixes #188

@ardecvz
Copy link
Contributor Author

ardecvz commented Sep 11, 2023

@palkan The extended test conformance output is fixed there - anycable/anyt#7
Anyway, I'm looking into the failures.

@ardecvz
Copy link
Contributor Author

ardecvz commented Sep 11, 2023

Locally no errors with NATS and NATS Embedded

@ardecvz
Copy link
Contributor Author

ardecvz commented Sep 11, 2023

It seems that the hidden errors are timeouts - Anyt::Client::TimeoutError
I've reproduced errors locally with another conformance suite (https), but it seems to be broken in the master too :'(

@palkan
Copy link
Member

palkan commented Sep 12, 2023

Locally no errors with NATS and NATS Embedded

Have you tried upgrading nats-pure?) (I guess, @Envek knows what happened 😁)

UPD: nats-io/nats-pure.rb#132

@palkan
Copy link
Member

palkan commented Sep 12, 2023

Thanks! I pinned nats-pure to an older version for now. Merging this and hope all tests will pass now.

@palkan palkan merged commit 1968400 into anycable:master Sep 12, 2023
13 of 21 checks passed
@palkan
Copy link
Member

palkan commented Sep 12, 2023

Okay, this is a real one: https://github.com/anycable/anycable-go/actions/runs/6154293794/job/16699510933

@ardecvz Could you please take a look? (And propose a follow-up PR?)

@ardecvz ardecvz deleted the feature/database-number branch September 12, 2023 06:13
@ardecvz
Copy link
Contributor Author

ardecvz commented Sep 12, 2023

Of course, NATS hides these failures.
And did you see the WARNING in the PR description?
This change may break user setups when we're starting to use the proper database number - is it OK to lose data remnants there?

@palkan
Copy link
Member

palkan commented Sep 12, 2023

is it OK to lose data remnants there?

Yeah, it's fine. All the data we store has TTL.

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.

Anycable-go ignores Redis database number
2 participants