-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
VTAdmin: Support for conclude txn and abandon age param #16834
VTAdmin: Support for conclude txn and abandon age param #16834
Conversation
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16834 +/- ##
==========================================
- Coverage 69.43% 69.41% -0.03%
==========================================
Files 1570 1570
Lines 203812 203846 +34
==========================================
- Hits 141517 141496 -21
- Misses 62295 62350 +55 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Noble Mittal <[email protected]>
@@ -78,7 +78,7 @@ func (r Request) ParseQueryParamAsUint32(name string, defaultVal uint32) (uint32 | |||
} | |||
|
|||
// ParseQueryParamAsInt32 attempts to parse the query parameter of the given | |||
// name into a uint32 value. If the parameter is not set, the provided default | |||
// name into a int32 value. If the parameter is not set, the provided default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo :)
Signed-off-by: Noble Mittal <[email protected]>
go/vt/vtadmin/api.go
Outdated
if !api.authz.IsAuthorized(ctx, c.ID, rbac.KeyspaceResource, rbac.GetAction) { | ||
return nil, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this would be better as a:
if !api.authz.IsAuthorized(ctx, c.ID, rbac.ClusterResource, rbac.GetAction) {
return nil, nil
}
Before fetching the cluster on line 539 instead!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -23,13 +23,32 @@ import ( | |||
) | |||
|
|||
// GetUnresolvedTransactions implements the http wrapper for the | |||
// /transactions/{cluster_id}/{keyspace} route. | |||
// /transactions/{cluster_id}/{keyspace}[?abandon_age=] route. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳
transactionsQuery.refetch(); | ||
} | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, [params.abandonAge]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wouldn't you want to update on params clusterID
and keyspace
changing too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already using params
(which is a stateful value, contains keyspace
, clusterID
and abandonAge
) while fetching transactions.
const transactionsQuery = useTransactions(params, {
enabled: !!params.keyspace,
});
Infact, we don't even need useEffect
here. 😃 Removing it.
Signed-off-by: Noble Mittal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this fall off my radar.
We should keep the default set for Abandon Age to 15 mins here.
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Description
abandonAge
, if it's empty, we use the default abandon age.5sec
,30sec
,1min
,5min
,15min
and1hr
.Related Issue(s)
Screenshots
Checklist
Deployment Notes