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

OCPBUGS-38284: v2 clean up subscriptions #77

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

jzding
Copy link
Member

@jzding jzding commented Aug 14, 2024

No description provided.

@jzding jzding force-pushed the v2-sub-cleanup branch 6 times, most recently from 9479470 to ba67c5b Compare August 15, 2024 14:21
@jzding jzding changed the title v2 clean up subscriptions OCPBUGS-38284: v2 clean up subscriptions Aug 15, 2024
numSubDeleted, err := s.subscriberAPI.DeleteAllSubscriptions()

// Subscriptions could be partially deleted when there were errors
if numSubDeleted > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

This will be true when all subscriptions are deleted too right? numSubDeleted equals the number of subscriptions deleted.. you need a way to check whether the number of subscriptions before the delete == numSubDeleted? if not you can update the metrics with the difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

The metric simple pegs number of subs what were deleted, either all (err==nil) or some (when err!=nil).

nishant-parekh
nishant-parekh previously approved these changes Aug 15, 2024
Copy link
Member

@nishant-parekh nishant-parekh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Signed-off-by: Jack Ding <[email protected]>
@jzding jzding merged commit d6f8679 into redhat-cne:main Aug 16, 2024
2 checks passed
jzding added a commit to jzding/rest-api that referenced this pull request Aug 29, 2024
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.

2 participants