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

Improve map deletion from Wan status #615

Merged
merged 2 commits into from Feb 20, 2023
Merged

Improve map deletion from Wan status #615

merged 2 commits into from Feb 20, 2023

Conversation

mtyazici
Copy link
Contributor

@mtyazici mtyazici commented Feb 6, 2023

Description

Improved map deletion from Wan status when publisher ID was empty. Also improved logs for easier debugging.

User Impact

@mtyazici mtyazici added the enhancement New feature or request label Feb 6, 2023
@mtyazici mtyazici added this to the 5.7 milestone Feb 6, 2023
@mtyazici mtyazici requested a review from a team as a code owner February 6, 2023 07:36
@mtyazici mtyazici requested review from SeriyBg and cagric0 and removed request for a team February 6, 2023 07:36
@mtyazici mtyazici temporarily deployed to report February 6, 2023 08:02 — with GitHub Actions Inactive
@mtyazici mtyazici temporarily deployed to report February 6, 2023 08:35 — with GitHub Actions Inactive
@mtyazici mtyazici temporarily deployed to report February 6, 2023 09:25 — with GitHub Actions Inactive
Copy link
Contributor

@cagric0 cagric0 left a comment

Choose a reason for hiding this comment

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

I just have one suggestion. Other than that, LGTM! After you address or give an answer, I'll approve.

Comment on lines +207 to +215
for mapWanKey := range wan.Status.WanReplicationMapsStatus {
m, ok := publishedMaps[mapWanKey]
if !ok {
// Delete map without publisherID from status
if err := deleteWanMapStatus(ctx, r.Client, wan, mapWanKey); err != nil {
return err
}
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you remove the map with empty publisherId from the status in the stopWanReplicationMap, I think we don't need to change here and an additional check for publisherId

Copy link
Contributor Author

@mtyazici mtyazici Feb 16, 2023

Choose a reason for hiding this comment

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

Yes, but how are we going to generate a client for that map? Client is a parameter for stopWanReplicationMap function. Map and Hazelcast CR might have been deleted already.

@mtyazici mtyazici merged commit 510df1d into main Feb 20, 2023
@mtyazici mtyazici deleted the wan-fix branch February 20, 2023 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants