[Dataflow Streaming] Update windmill timer clasification logic to work with windmill state tag encoding v2#37604
[Dataflow Streaming] Update windmill timer clasification logic to work with windmill state tag encoding v2#37604arunpandianp wants to merge 1 commit intoapache:masterfrom
Conversation
with windmill state tag encoding v2
|
R: @scwhittle |
Summary of ChangesHello @arunpandianp, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue in Dataflow Streaming where timers were failing to fire due to an outdated classification logic tied to Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
| WindmillTimerType windmillTimerType, TimerData timerData, ByteString timerTag); | ||
|
|
||
| /** | ||
| * Produce a tag that is guaranteed to be unique for the given prefix, namespace, domain and |
| if ("".equals(timerData.getTimerFamilyId())) { | ||
| tagString = | ||
| prefix.byteString().toStringUtf8() | ||
| windmillTimerType.namespacePrefix().toStringUtf8() |
There was a problem hiding this comment.
could have namespacePrefixString method on the timertype to avoid doing this same conversion many times
| "Expected timer tag %s to start with prefix %s", | ||
| tag, | ||
| prefix.byteString()); | ||
| ByteString tag = ByteString.copyFrom(timer.getTag().asReadOnlyByteBuffer()); |
There was a problem hiding this comment.
if we have teh namespaceprefixstring method could we avoid this copy to bytestring?
| timerTypeToTimeDomain(timer.getType())); | ||
| return Pair.of( | ||
| timerType, | ||
| TimerData.of( |
There was a problem hiding this comment.
how about putting the timertype in TimerData? The timedomain could be a function instead to keep the size same but then it is one less allocation than this Pair
| windowCoder, | ||
| getDrainMode())) | ||
| timer, windowCoder, getDrainMode())) | ||
| .filter(pair -> pair.getLeft() == WindmillTimerType.SYSTEM_TIMER) |
There was a problem hiding this comment.
can this remain above the transform since it's cheaper?
| windowCoder, | ||
| getDrainMode())) | ||
| timer, windowCoder, getDrainMode())) | ||
| .filter(pair -> pair.getLeft() == WindmillTimerType.USER_TIMER) |
| public WindmillTimerInternals( | ||
| String stateFamily, // unique identifies a step | ||
| WindmillNamespacePrefix prefix, // partitions user and system namespaces into "/u" and "/s" | ||
| WindmillTimerType type, // partitions user and system namespaces into "/u" and "/s" |
There was a problem hiding this comment.
remove the /u and /s specific comment?
| }; | ||
|
|
||
| public abstract ByteString byteString(); | ||
| public abstract ByteString namespacePrefix(); |
There was a problem hiding this comment.
maybe this should not be a method on the enum but static function within the v1 tag class?
That enforces that elsewhere we are always using the encoding agnostic type
|
|
||
| @Parameter(1) | ||
| public WindmillNamespacePrefix prefix; | ||
| public WindmillTimerType prefix; |
Timers were not firing due to the filtering which was based on encoding v1. Also adding a post submit test to catch such bugs early in #37603
The feature is not enabled anywhere yet and the bug does not affect real workloads.