Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Workaround for query-consistency-strong which is presently partially broken #5928

Merged
merged 6 commits into from
Apr 24, 2024
Merged

Workaround for query-consistency-strong which is presently partially broken #5928

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f9a77c7
Adding a quick workaround for now
davidporter-id-au Apr 23, 2024
98e2775
fix phrasing
davidporter-id-au Apr 23, 2024
800c025
fix overly long lines
davidporter-id-au Apr 23, 2024
bf17390
lint
davidporter-id-au Apr 23, 2024
4944c5d
Merge branch 'master' into bugfix/unblock-queries
davidporter-id-au Apr 23, 2024
1fa99db
Merge branch 'master' into bugfix/unblock-queries
davidporter-id-au Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions common/client/versionChecker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ func (vc *versionChecker) SupportsStickyQuery(clientImpl string, clientFeatureVe
// SupportsConsistentQuery returns error if consistent query is not supported otherwise nil.
// In case client version lookup fails assume the client does not support feature.
func (vc *versionChecker) SupportsConsistentQuery(clientImpl string, clientFeatureVersion string) error {

return vc.featureSupported(clientImpl, clientFeatureVersion, consistentQuery)
}

Expand Down
10 changes: 9 additions & 1 deletion service/history/decision/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,15 @@ func (handler *handlerImpl) handleBufferedQueries(

// Consistent query requires both server and client worker support. If a consistent query was requested (meaning there are
// buffered queries) but worker does not support consistent query then all buffered queries should be failed.
if versionErr := handler.versionChecker.SupportsConsistentQuery(clientImpl, clientFeatureVersion); versionErr != nil {
versionErr := handler.versionChecker.SupportsConsistentQuery(clientImpl, clientFeatureVersion)
// todo (David.Porter) remove the skip on version check for
// clientImpl and clientFeatureVersion where they're nil
// There's a bug, probably in matching somewhere which isn't
// forwarding the client headers for version
// info correctly making this call erroneously fail sometimes.
// https://t3.uberinternal.com/browse/CDNC-8641
// So defaulting just this flow to fail-open in the absence of headers.
if versionErr != nil && clientImpl != "" && clientFeatureVersion != "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm sure, this is fine for now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are code paths within vesionChecker that handles clientImpl == nil gracefully. so expecting both clientImpl and clientFeatureVersion to be empty might exclude some genuine cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed offline, I believe we concluded that the clients that didn't support this and were using QueryConsistencyStrong to be a small set.

scope.IncCounter(metrics.WorkerNotSupportsConsistentQueryCount)
failedTerminationState := &query.TerminationState{
TerminationType: query.TerminationTypeFailed,
Expand Down
Loading