Remove ExecutableWrapper #5033
Merged
Remove ExecutableWrapper #5033
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yycptt
reviewed
Oct 25, 2023
MichaelSnowden
force-pushed
the
snowden/no-wrapper
branch
from
f276a6b
to
e0ca667
Compare
yycptt
approved these changes
Oct 25, 2023
MichaelSnowden
added a commit
that referenced
this pull request
Feb 14, 2024
## What changed? <!-- Describe what has changed in this PR --> I added back some metrics that were removed from DLQ writes when we removed the Executable DLQ Wrapper in #5033 ## Why? <!-- Tell your future self why have you made these changes --> These were removed by mistake. ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> The more important metric, "dlq_writes" still exists, and that's what the docs reference for operators, so we're ok that these other ones were missed. ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
I discovered an issue with the
ExecutableWrapperhere. Because we don't wrap theReschedulemethod, it adds the unwrapped version to the rescheduler, which loses all of the wrapped behavior. I implemented a fix using the "delegate" pattern, where the base instance holds a reference to the wrapped instance, but it is fraught with peril: you need to remember to use the delegate everywhere it's applicable, and you need to remember to set it--also only one delegate can be set, so you can't have different wrappers for the same object. As a result, I decided to get rid of the wrapper-based approach, and instead added the code directly to the base executable implementation.Why?
The huge refactoring is necessary because the
ExecutableWrapperapproach is too complicated and risky.How did you test it?
There's still 100% test coverage for the DLQ code. Most imporantly, there is now an end-to-end integration test which starts a workflow that produces terminally failing tasks, verifies that it's added to the DLQ, and then uses the tdbg read command to delete the DLQ tasks. This wasn't possible before due to the bug in the
ExecutableWrapper.Potential risks
Is hotfix candidate?