-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 multiple current attributes in middleware #5904
Conversation
But why do you have multiple? |
And your suggestion supports 2. What if I want 3? |
In our case, we need to handle global state for two separate concerns, each with their own
The proposed solution supports an arbitrary number of Sidekiq::CurrentAttributes.persist({
cattr: "Myapp::Current",
cattr_other: "Myapp::OtherCurrent",
cattr_three: "Myapp::ThirdCurrent",
cattr_fourth: "Myapp::FourthCurrent",
...
}) |
I understand your usecase. I don't like making the user configure the attribute name. I'm wondering if it would be simpler to do something like: Sidekiq::CurrentAttributes.persist("Myapp::Current", "Myapp::OtherCurrent") The first would be stored in WDYT? |
That was the main reason I proposed the user defined key, but maybe documenting the caveat about changing order is good enough. Do you prefer the suggested variable length method signature over accepting an array in the first position? We currently have the signature below, and we would need to type check the last argument to see if it's a config of a class string. def persist(klass, config = Sidekiq.default_configuration) Let me know how to proceed, and I'll make the changes. Thanks! |
I think a single klass_or_array argument would be my initial thought but push back if there’s a good reason to. |
b432026
to
1f1ec73
Compare
1f1ec73
to
da4ccc2
Compare
Made the suggested updates. Please, let me know if it's acceptable for merging now or if further tweaks are needed. Thanks! |
Currently, when calling
Sidekiq::CurrentAttributes.persist(...)
for multiple current attributes, the middleware doesn't correctly load the serialized data in the server middleware. In fact, it fails because the attributes from the first current attribute are used for "rehydrating" the second one.The backwards compatible solution in this PR allows for a hash argument to be passed to the aforementioned method, with a key that is used in the
jobs
hash.