Skip to content

Conversation

@rdhabalia
Copy link
Contributor

Motivation

This PR is on top of #4458. It adds support to delete bookie-affinity group using cli/admin api.

@rdhabalia rdhabalia added this to the 2.4.0 milestone Jun 4, 2019
@rdhabalia rdhabalia self-assigned this Jun 4, 2019
@rdhabalia
Copy link
Contributor Author

retest this please

@apache apache deleted a comment from marutronix Jun 6, 2019
Fix: get correct available-primary bookie count | cli optional

fix test
@rdhabalia
Copy link
Contributor Author

rerun integration tests

Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

@Anonymitaet @jennifer88huang can anyone of you help review the documentation for this api endpoint?

@ApiOperation(hidden = true, value = "Delete the bookie-affinity-group from namespace-local policy.")
@ApiResponses(value = { @ApiResponse(code = 403, message = "Don't have admin permission"),
@ApiResponse(code = 404, message = "Namespace does not exist"),
@ApiResponse(code = 409, message = "Concurrent modification") })
Copy link
Member

Choose a reason for hiding this comment

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

@rdhabalia since you are adding new endpoints, do you mind making sure all the return codes are properly documented? since there are multiple efforts on improving the documentation around this area.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it seems correct to me.

@DELETE
@Path("/{property}/{namespace}/persistence/bookieAffinity")
@ApiOperation(hidden = true, value = "Delete the bookie-affinity-group from namespace-local policy.")
@ApiResponses(value = { @ApiResponse(code = 403, message = "Don't have admin permission"),
Copy link
Member

Choose a reason for hiding this comment

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

same comment as above

Copy link
Contributor

@Jennifer88huang-zz Jennifer88huang-zz left a comment

Choose a reason for hiding this comment

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

I have checked description, and there is no issue with it.
Approve from language level. Dev can check the methods and other tech issues.

@codelipenghui
Copy link
Contributor

@rdhabalia do we need this issue for 2.4.0? or can we move it to 2.5.0?

@rdhabalia
Copy link
Contributor Author

yes, we need this one for 2.4 and it's ready to merge as well.

@sijie
Copy link
Member

sijie commented Jun 13, 2019

@rdhabalia can you check the review comments from @jennifer88huang and @Anonymitaet ?

@rdhabalia
Copy link
Contributor Author

@sijie

can you check the review comments from @jennifer88huang and @Anonymitaet ?

Sure, but I replied to comment and it seems things are expected. Can you please let me know if I am missing anything here to address.

@rdhabalia
Copy link
Contributor Author

rerun java8 tests
rerun cpp tests

1 similar comment
@rdhabalia
Copy link
Contributor Author

rerun java8 tests
rerun cpp tests

@apache apache deleted a comment from rdhabalia Jun 14, 2019
@rdhabalia
Copy link
Contributor Author

rerun cpp tests

@sijie sijie merged commit 6636c79 into apache:master Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants