-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
NIFI-4733: Addressing two phase commit logic issue in variable registry #2370
Conversation
Reviewing... |
@@ -552,41 +553,43 @@ public Response deleteVariableRegistryUpdateRequest( | |||
public Response updateVariableRegistry( | |||
@Context final HttpServletRequest httpServletRequest, | |||
@ApiParam(value = "The process group id.", required = true) @PathParam("id") final String groupId, | |||
@ApiParam(value = "The process group configuration details.", required = true) final VariableRegistryEntity requestEntity) { | |||
@ApiParam(value = "The process group configuration details.", required = true) final VariableRegistryEntity requestVariableRegistryEntity) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the ApiParam
value be updated to reflect that this is VR-specific as opposed to generic PG configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes good catch.
@@ -620,13 +623,13 @@ public Response updateVariableRegistry( | |||
public Response submitUpdateVariableRegistryRequest( | |||
@Context final HttpServletRequest httpServletRequest, | |||
@ApiParam(value = "The process group id.", required = true) @PathParam("id") final String groupId, | |||
@ApiParam(value = "The process group configuration details.", required = true) final VariableRegistryEntity requestEntity) { | |||
@ApiParam(value = "The process group configuration details.", required = true) final VariableRegistryEntity requestVariableRegistryEntity) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably here too, then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup.
I ran a cluster with the patch applied and ensured no regressions on the two phase commit. Once the minor fixes are done, I can merge, but I'll leave the pre-emptive +1 here in case I am away from my computer. Ran |
- Resolving logic issue in two phase commit when updating variable registry. This closes apache#2370
Thanks @alopresto! I've addressed the comments here and I will merge to master. |
NIFI-4733: