Deprecate timer builder #2725
Merged
Deprecate timer builder #2725
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
Contributor
wxing1292
commented
Oct 28, 2019
•
wxing1292
commented
- Deprecate timer builder
- Rename TimerSequenceID to timerKey
- Do not expose user timer task / activity timer task vars
- Rewrite time active processing logic UT
* Rename TimerSequenceID to timerKey * Do not expose user timer task / activity timer task vars TODO: * Rename DecisionTimeoutValue to DecisionStartToCloseTimeout * DecisionTimeout in decisionInfo is always DecisionTimeoutValue or 0? * Rewrite timer active processing logic * Mutable state should have the ability to tell whether a write to DB can be omitted
wxing1292
force-pushed
the
deprecate-timer-builder
branch
from
1dfb05e
to
00036ba
Compare
wxing1292
force-pushed
the
deprecate-timer-builder
branch
from
99ffd2d
to
12ea40c
Compare
yux0
approved these changes
Oct 30, 2019
yycptt
approved these changes
Oct 31, 2019
meiliang86
reviewed
Oct 31, 2019
| if err != nil { | ||
| return nil, err | ||
| } | ||
| targetDomainID = targetDomainEntry.GetInfo().ID |
There was a problem hiding this comment.
What is the reason to override domain in this case? Can you put a comment in the code?
| } | ||
|
|
||
| func (r *mutableStateTaskGeneratorImpl) getTimerBuilder(now time.Time) *timerBuilder { | ||
| func (r *mutableStateTaskGeneratorImpl) getTimerSequence(now time.Time) timerSequence { |
There was a problem hiding this comment.
Any way that this can belong to mutable state?
|
|
||
| // timerKey - Visibility timer stamp + Sequence Number. | ||
| timerKey struct { | ||
| VisibilityTimestamp time.Time |
There was a problem hiding this comment.
You must have got used to this. But it's not very clear to me what visibility timestamp means for a timer. Worth putting a comment there.
There was a problem hiding this comment.
My guess is it's the timestamp the timer is supposed to fire?
| @@ -228,30 +228,33 @@ func (t *timerQueueStandbyProcessorImpl) process( | |||
| } | |||
| } | |||
|
|
|||
| func (t *timerQueueStandbyProcessorImpl) processExpiredUserTimer( | |||
| func (t *timerQueueStandbyProcessorImpl) processUserTimerTimeout( | |||
There was a problem hiding this comment.
I think processExpiredUserTimer is a better name, since you still check timerSequence.isExpired inside.
| for _, timerSequenceID := range timerSequence.loadAndSortUserTimers() { | ||
| _, ok := mutableState.GetUserTimerInfoByEventID(timerSequenceID.eventID) | ||
| if !ok { | ||
| errString := fmt.Sprintf("failed to find in user timer event ID: %v", timerSequenceID.eventID) |
There was a problem hiding this comment.
Why remove "memory"? :)
| timerTaskStatusCreatedScheduleToClose | ||
| timerTaskStatusCreatedHeartbeat | ||
| ) | ||
|
|
||
| type ( | ||
| // timerSequenceID | ||
| timerSequenceID struct { |
There was a problem hiding this comment.
This name is a little weird to me.
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.