[BEAM-12333] Changing TimerKey to include TimerFamilyId #14802
Conversation
ajothomas
changed the title
| // encode a byte (1/0) to indicate the presence/absence of timerFamilyId | ||
| // We can use this approach to add additional fields in the future | ||
| if (!Strings.isNullOrEmpty(value.getTimerFamilyId())) { | ||
| BOOLEAN_CODER.encode(true, outStream); |
There was a problem hiding this comment.
could you help me understand why do we need a boolean flag? I suppose inStream.available() > 0 could be sufficient to indicate there will be extra fields.
There was a problem hiding this comment.
This was just for future proofing in the event we add more fields to TimerKey. TimerFamilyId is optional and in the case it is null/empty, I wanted to encode a flag to indicate the absence of a timer family id. If we add more fields in the future, we can easily decode in a backward compatible way using the flag.
The only way we can probably omit encoding this boolean flag is to encode an empty string ("") whenever family id is not present.
There was a problem hiding this comment.
Got you, actually we do not need that because when timer family id is not present, we will still encode null in the bytes to actually indicate that timer family id is null.
There was a problem hiding this comment.
@ajothomas : I think we should be able to blindly encode timeFamilyId as it's going to be defaulted as blank "". See TimerInternals: line 198. The new TimerDataCoder (TimerDataCoderV2) also encode it directly without the need to do null check. We should be able to follow that.
There was a problem hiding this comment.
Thanks for catching this. As long as it defaults to empty string, StringUtf8Coder will be able to encode/decode it. Will remove the boolean flag.
| // encode a byte (1/0) to indicate the presence/absence of timerFamilyId | ||
| // We can use this approach to add additional fields in the future | ||
| if (!Strings.isNullOrEmpty(value.getTimerFamilyId())) { | ||
| BOOLEAN_CODER.encode(true, outStream); |
There was a problem hiding this comment.
@ajothomas : I think we should be able to blindly encode timeFamilyId as it's going to be defaulted as blank "". See TimerInternals: line 198. The new TimerDataCoder (TimerDataCoderV2) also encode it directly without the need to do null check. We should be able to follow that.
| // check if the stream has more available bytes. This is to ensure backward compatibility with | ||
| // old rocksdb state | ||
| // which does not encode timer family data | ||
| if (inStream.available() > 0 && BOOLEAN_CODER.decode(inStream)) { |
There was a problem hiding this comment.
As commented above, we can ignore this bool flag.
ajothomas
force-pushed
the
TimerKeyChanges
branch
3 times, most recently
from
c1c5ecc
to
48c17e4
Compare
|
|
||
| return new TimerKey<>(key, namespace, timerId); | ||
| return timerKeyBuilder.build(); |
There was a problem hiding this comment.
nit: we do not need to declare the builder at the begining
we can do
return TimerKey.builder().setX().setY().setZ().build();
There was a problem hiding this comment.
RIght, it seems nicer to have the builder in the end instead of cutting it in half.
| } | ||
|
|
||
| STRING_CODER.encode(value.getTimerFamilyId(), outStream); |
There was a problem hiding this comment.
I guess the answer is Yes but just to double check whether we want to encode it if timer family id is an empty string.
ajothomas
force-pushed
the
TimerKeyChanges
branch
from
48c17e4
to
3eeb6bc
Compare
|
Hi there, this PR breaks https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/. I just filed https://issues.apache.org/jira/browse/BEAM-12358 for tracking and please consider either reverting this change or having a fix. |
|
thanks, @boyuanzz, we are looking into it. |
|
Thank you : ) |
SamzaTimerInternalsFactorystores timer state, event time and processing time timers alike, in the form of aMapState<TimerKey<K>, Long>.TimerKey, however, doesn't include theTimerFamilyIdand therefore the timer family information is not persisted. On the other hand, event time timers uses an additional time sorted set to persist the TimerData(which contains TimerFamilyId). We need to include timer family id in the TimerKey to ensure that it is persisted for Processing time timers.This PR aims to add
TimerFamilyIdtoTimerKey. Additionally, I am switching to the use ofAutoValueto reduce boilerplate code.ValidatesRunnercompliance status (on master branch)Examples testing status on various runners
Post-Commit SDK/Transform Integration Tests Status (on master branch)
SamzaTimerInternalsFactorystores timer state, event time and processing time timers alike, in the form of a MapState<TimerKey, Long>. TimerKey, however, doesn't include theTimerFamilyIdand is therefore not persisted. On the other hand, event time timers uses an additional time sorted set to persist the TimerData(which contains TimerFamilyId). We need to include timer family id in the TimerKey to ensure that it is persisted.Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.