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

Don't delete group if there is no member in the group #4274

Merged
merged 4 commits into from
Nov 21, 2019

Conversation

poonai
Copy link
Contributor

@poonai poonai commented Nov 14, 2019

Manually tested by having two alpha with 1 replica count. removed one alpha and joined a new alpha. Alpha joined back to the existing group as expected
Signed-off-by: பாலாஜி [email protected]


This change is Reviewable

@poonai poonai requested review from manishrjain and a team as code owners November 14, 2019 10:01
Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

The group delete test is not passing anymore (since it's testing explicitly that groups are deleted). I suppose that test is obsolete if this PR is approved.

But you should write a similar test (or change the group delete test) to verify that groups are not deleted.

Also, what would happen to tablets that are assigned to an empty group? We should verify that empty groups are not assigned new tablets and that the existing tablets of a group with no members are removed before the group is empty (this might already be done when removing a group but I am not sure).

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @manishrjain)

@poonai
Copy link
Contributor Author

poonai commented Nov 15, 2019

@martinmr good catch about tablet. Lemme check and verify

பாலாஜி added 2 commits November 15, 2019 16:13
Signed-off-by: பாலாஜி <[email protected]>
Signed-off-by: பாலாஜி <[email protected]>
Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Looks good to me. Leave it to @martinmr to approve.

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

:lgtm:

One comment regarding the test but feel free to merge once it's been addressed.

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @balajijinnah)


systest/group-delete/group_delete_test.go, line 161 at r2 (raw file):

Quoted 4 lines of code…
	group, ok := state2.Groups["3"]
	if !ok {
		t.Errorf("group 3 is removed")
	}

Can you change this so we use the require library everywhere?

so this would become.

group, ok := state2.Groups["3"]
require.False(ok, "group 3 is removed")

Same for all the other instances of t.Errorf in this test.

Signed-off-by: பாலாஜி <[email protected]>
Copy link
Contributor Author

@poonai poonai left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @manishrjain and @martinmr)


systest/group-delete/group_delete_test.go, line 161 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…
	group, ok := state2.Groups["3"]
	if !ok {
		t.Errorf("group 3 is removed")
	}

Can you change this so we use the require library everywhere?

so this would become.

group, ok := state2.Groups["3"]
require.False(ok, "group 3 is removed")

Same for all the other instances of t.Errorf in this test.

Done.

@poonai poonai merged commit 61bd23e into master Nov 21, 2019
@poonai poonai deleted the balaji/group_remove branch November 21, 2019 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants