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.

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

Add namespace_delete_delay option to DeleteNamespaceRequest #343

Merged
merged 3 commits into from
Jan 12, 2024

Conversation

hehaifengcn
Copy link
Contributor

@hehaifengcn hehaifengcn commented Jan 11, 2024

What changed?

Why?
So we can override DeleteNamespaceNamespaceDeleteDelay dynamic config setting for deleting namespace info.

Breaking changes

@hehaifengcn hehaifengcn marked this pull request as ready for review January 12, 2024 20:56
@hehaifengcn hehaifengcn requested review from a team as code owners January 12, 2024 20:56
@@ -74,6 +75,9 @@ message DeleteNamespaceRequest {
// Only one of namespace or namespace_id must be specified to identify namespace.
string namespace = 1;
string namespace_id = 2;
// If provided, the deletion of namespace info will be delayed for given duration (0 means no delay).
// If not provided, use default delay configured in the cluster.
google.protobuf.Duration namespace_delete_delay = 3;
Copy link
Member

Choose a reason for hiding this comment

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

It is already in DeleteNamespaceRequest. I would call it just delay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

delay may cause confusion that the whole deletion was delayed, which isn't the case (WF will be still deleted immediately).

Comment on lines 78 to 79
// If provided, the deletion of namespace info will be delayed for given duration (0 means no delay).
// If not provided, use default delay configured in the cluster.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// If provided, the deletion of namespace info will be delayed for given duration (0 means no delay).
// If not provided, use default delay configured in the cluster.
// If provided, the deletion of namespace info will be delayed for given duration (0 means no delay).
// If not provided, the default delay configured in the cluster will be used.

Copy link

@mjameswh mjameswh left a comment

Choose a reason for hiding this comment

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

LGTM

@hehaifengcn hehaifengcn enabled auto-merge (squash) January 12, 2024 21:31
@hehaifengcn hehaifengcn merged commit 66e0fff into master Jan 12, 2024
4 checks passed
@hehaifengcn hehaifengcn deleted the haifengh/ns-delete-delay branch January 12, 2024 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants