Wire up dynamic config-backed history DLQ #4876
Conversation
service/history/dlq.go
Outdated
| return &executableToggle{ | ||
| Executable: e, | ||
| executableDLQ: executableDLQ, | ||
| useDLQ: d.DC.GetBoolProperty(dynamicconfig.HistoryTaskDLQEnabled, false), |
There was a problem hiding this comment.
nit: plz define it in history/configs.Config
There was a problem hiding this comment.
There's a lot of variables in the history/configs.Config object that aren't required by this object, and it seems to be another property bag. Having the DC client where we can specify only the key for the value we need seems like better information-hiding to me. Is there a reason we want to use the Config instead?
There was a problem hiding this comment.
syned offline. Idea is not pass the entire config struct in but define dc functions in a central place so that func can be reused and also easier to find default values for all defined configs. The struct should still take in one config.
service/history/dlq.go
Outdated
| executableToggle struct { | ||
| queues.Executable | ||
| executableDLQ *queues.ExecutableDLQ | ||
| useDLQ dynamicconfig.BoolPropertyFn | ||
| } |
There was a problem hiding this comment.
do you plan to use this struct for other functionalities in the future? If so, it should probably live in a dedicated file, instead of dlq.go. If no, we can move the dc into ExecuableDLQ or rename to DLQToggle?
There was a problem hiding this comment.
I want this to be specific to the main entry point because the other unit tests don't need this. Basically, in general, I think feature flags should be decoupled from the feature itself and just wrap them in the main entry point so that we don't end up with a bunch of places in tests where we have to do something like dcOverrides: map[string]interface{}{ dynamicconfig.HistoryTaskDLQEnabled: true }. I also think this should only be a temporary thing because we probably want the DLQ on always once it's tested.
There was a problem hiding this comment.
synced offline, plz rename to executableDLQToggle or something similar.
MichaelSnowden
force-pushed
the
snowden/dlq-5-base
branch
from
688bca0
to
1ca1bc4
Compare
MichaelSnowden
force-pushed
the
snowden/dlq-5
branch
from
bb8dbac
to
73c1363
Compare
MichaelSnowden
force-pushed
the
snowden/dlq-5-base
branch
from
1ca1bc4
to
4682ed8
Compare
MichaelSnowden
force-pushed
the
snowden/dlq-5
branch
from
73c1363
to
5804a83
Compare
service/history/dlq.go
Outdated
| executableToggle struct { | ||
| queues.Executable | ||
| executableDLQ *queues.ExecutableDLQ | ||
| useDLQ dynamicconfig.BoolPropertyFn | ||
| } |
There was a problem hiding this comment.
synced offline, plz rename to executableDLQToggle or something similar.
service/history/dlq.go
Outdated
| return &executableToggle{ | ||
| Executable: e, | ||
| executableDLQ: executableDLQ, | ||
| useDLQ: d.DC.GetBoolProperty(dynamicconfig.HistoryTaskDLQEnabled, false), |
There was a problem hiding this comment.
syned offline. Idea is not pass the entire config struct in but define dc functions in a central place so that func can be reused and also easier to find default values for all defined configs. The struct should still take in one config.
MichaelSnowden
force-pushed
the
snowden/dlq-5
branch
from
5804a83
to
c8ccf1e
Compare
MichaelSnowden
changed the base branch from
snowden/dlq-5-base
to
snowden/dlq-feature
<!-- Describe what has changed in this PR --> **What changed?** This is the final PR to actually wire up the history task DLQ. _Note about the PR structure: The merge base is dlq-5-base, but it will be changed to dlq-feature after #4875 is merged. I did it this way because 4875 is a PR against main (because it's independent of the DLQ project), but this PR depends on both that and another PR in the dlq-feature branch._ <!-- Tell your future self why have you made these changes --> **Why?** I made this so that we can actually use the DLQ. All previous PRs were basically library code--this hooks it up to our main entry point. <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> **How did you test it?** There's an fx test which verifies that any executable we create will use the DLQ iff the dynamic config flag is on. <!-- Assuming the worst case, what can be broken when deploying this change to production? --> **Potential risks** The dynamic config defaults to false, and I added a comment to not turn it on if you aren't using Cassandra. <!-- Is this PR a hotfix candidate or require that a notification be sent to the broader community? (Yes/No) --> **Is hotfix candidate?** No.
What changed?
This is the final PR to actually wire up the history task DLQ.
Note about the PR structure: The merge base is dlq-5-base, but it will be changed to dlq-feature after #4875 is merged. I did it this way because 4875 is a PR against main (because it's independent of the DLQ project), but this PR depends on both that and another PR in the dlq-feature branch.
Why?
I made this so that we can actually use the DLQ. All previous PRs were basically library code--this hooks it up to our main entry point.
How did you test it?
There's an fx test which verifies that any executable we create will use the DLQ iff the dynamic config flag is on.
Potential risks
The dynamic config defaults to false, and I added a comment to not turn it on if you aren't using Cassandra.
Is hotfix candidate?
No.