Added 2 more tags in log for comparator to use. #5646
Conversation
bowenxia
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| @@ -51,6 +51,14 @@ func Timestamp(timestamp time.Time) Tag { | |||
| return newTimeTag("timestamp", timestamp) | |||
| } | |||
|
|
|||
| func EarliestTime(time int64) Tag { | |||
There was a problem hiding this comment.
if I may suggest making it *int65
| func EarliestTime(time int64) Tag { | |
| func EarliestTime(time *int64) Tag { | |
| if time == nil { | |
| return newInt64("earliest-time", -1) | |
| } | |
| return newInt64("earliest-time", time) | |
| } |
| if userParam.earlistTime == nil && userParam.latestTime == nil { | ||
| v.logger.Info("Logging user query parameters for Pinot/ES response comparator with custom query...", |
There was a problem hiding this comment.
I'm nitpicking, but I'd suggest pulling the responsibility for handling nil behaviour into the tag (as per the tag discussion) and avoiding branching an if here
| @@ -452,10 +472,15 @@ func (v *pinotVisibilityTripleManager) GetClosedWorkflowExecution( | |||
| ctx context.Context, | |||
| request *GetClosedWorkflowExecutionRequest, | |||
| ) (*GetClosedWorkflowExecutionResponse, error) { | |||
| earlistTime := int64(0) // this is to get all closed workflow execution | |||
There was a problem hiding this comment.
did you want to hardcode these values?
There was a problem hiding this comment.
Yes. My understanding for GetClosedWorkflowExecutionRequest is to get all the closed workflow. Am I correct?
There was a problem hiding this comment.
I don't think so? Like this should be for a time range?
There was a problem hiding this comment.
Discussed offline, Bowen correctly points out that the request struct doesn't have a lower bound (for whatever reason), so this value is not used here.
| @@ -452,10 +472,15 @@ func (v *pinotVisibilityTripleManager) GetClosedWorkflowExecution( | |||
| ctx context.Context, | |||
| request *GetClosedWorkflowExecutionRequest, | |||
| ) (*GetClosedWorkflowExecutionResponse, error) { | |||
| earlistTime := int64(0) // this is to get all closed workflow execution | |||
There was a problem hiding this comment.
Discussed offline, Bowen correctly points out that the request struct doesn't have a lower bound (for whatever reason), so this value is not used here.
What changed?
Added 2 more tags in log for comparator to use.
Why?
We will need user's earliest time and latest time info in the response comparator.
Previously we didn't need it because we were testing the functionality of the comparator. In order to made it stable, we used our own hard coded earliest time and latest time. But this is not useful any more for the testing right now.
How did you test it?
manual testing
Potential risks
nah
Release notes
Documentation Changes