You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/01/08 12:35:59 UTC
[GitHub] [nifi] bejancsaba opened a new pull request #5648: NIFI-9428: Extending OperandType to Enum
bejancsaba opened a new pull request #5648:
URL: https://github.com/apache/nifi/pull/5648
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
Extending OperandType to Enum and connecting valid operands to operations in the C2 protocol API
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [x] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [x] Have you verified that the full build is successful on JDK 8?
- [ ] Have you verified that the full build is successful on JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mattyb149 commented on pull request #5648: NIFI-9428: Extending OperandType to Enum
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on pull request #5648:
URL: https://github.com/apache/nifi/pull/5648#issuecomment-1008083830
NIFI-9428 is the umbrella epic for all the C2 work. IMO it would be better to create an issue in the epic and have your PR reference that case, that way we can Resolve it once merged but leave the epic open until "the end". Also since NIFI-9428 has no separate commits AFAIK and this is an API change against something already in main, I recommend basing this PR against main. Once merged we can rebase the NIFI-9428 branch against main for when work begins on things like the client implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] bejancsaba commented on pull request #5648: NIFI-9428: Extending OperandType to Enum
Posted by GitBox <gi...@apache.org>.
bejancsaba commented on pull request #5648:
URL: https://github.com/apache/nifi/pull/5648#issuecomment-1008137872
> Thanks for the contribution @bejancsaba! The `OperandType` enum license header does not meet standard Apache License 2.0 guidelines, so that needs to be corrected before this PR can be approved. Running Maven build with the `contrib-check` profile should indicate whether the header matches the expected license terms.
>
> Other maintainers may have more substantive comments, but this change appears to make the `operand` property more restrictive, as opposed to the current `String` type. Will that restriction cause any problems for different versions of the C2 protocol?
Thank you for the guidance. I corrected the License issue and validated my change with contrib-check. Following up on the comment from @mattyb149 I'm closing this PR and opening a new one against main under a new dedicated ticket and with the change requested here.
As the [specification](https://cwiki.apache.org/confluence/display/MINIFI/C2+Design) is pretty specific about operation's and their valid operand(s) the idea was that with this deliberate restriction you could have "safer" implementations. I think we should be good with different c2 versions but I would appreciate if @kevdoran or @mattyb149 would take a look as they have more background and context here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] bejancsaba commented on pull request #5648: NIFI-9428: Extending OperandType to Enum
Posted by GitBox <gi...@apache.org>.
bejancsaba commented on pull request #5648:
URL: https://github.com/apache/nifi/pull/5648#issuecomment-1008138374
> NIFI-9428 is the umbrella epic for all the C2 work. IMO it would be better to create an issue in the epic and have your PR reference that case, that way we can Resolve it once merged but leave the epic open until "the end". Also since NIFI-9428 has no separate commits AFAIK and this is an API change against something already in main, I recommend basing this PR against main. Once merged we can rebase the NIFI-9428 branch against main for when work begins on things like the client implementation.
Thanks for the pointers I'm closing this one, created a dedicated ticket for this change under the epic: https://issues.apache.org/jira/browse/NIFI-9557 and will open a new PR against main as you suggested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5648: NIFI-9428: Extending OperandType to Enum
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5648:
URL: https://github.com/apache/nifi/pull/5648#discussion_r780683978
##########
File path: c2/c2-protocol/c2-protocol-api/src/main/java/org/apache/nifi/c2/protocol/api/OperandType.java
##########
@@ -0,0 +1,39 @@
+/*
+ * (c) 2018-2021 Cloudera, Inc. All rights reserved.
Review comment:
This license header does not meet the standard Apache License 2.0 header guidelines. Please see other files for examples.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] bejancsaba closed pull request #5648: NIFI-9428: Extending OperandType to Enum
Posted by GitBox <gi...@apache.org>.
bejancsaba closed pull request #5648:
URL: https://github.com/apache/nifi/pull/5648
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] kevdoran edited a comment on pull request #5648: NIFI-9428: Extending OperandType to Enum
Posted by GitBox <gi...@apache.org>.
kevdoran edited a comment on pull request #5648:
URL: https://github.com/apache/nifi/pull/5648#issuecomment-1009019180
Hi @bejancsaba @mattyb149
Regarding operand being an Enum vs String, I think the original intent of that field was to be a place for a target component id for operations that can target individual components, e.g., Processors. So something like a stop/start/restart-component operation that is targeting a single component in the flow rather than the entire flow could pass a component id as the operand. For this reason, String seems more suitable to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] kevdoran commented on pull request #5648: NIFI-9428: Extending OperandType to Enum
Posted by GitBox <gi...@apache.org>.
kevdoran commented on pull request #5648:
URL: https://github.com/apache/nifi/pull/5648#issuecomment-1009019180
Hi @bejancsaba @mattyb149
Regarding operand being an Enum vs String, I think the original intent of that field was to be a place for a target component id for operations that can target individual competes, e.g., Processors. So something like a stop/start/restart-component operation that is targeting a single component in the flow rather than the entire flow could pass a component id as the operand. For this reason, String seems more suitable to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] bejancsaba commented on pull request #5648: NIFI-9428: Extending OperandType to Enum
Posted by GitBox <gi...@apache.org>.
bejancsaba commented on pull request #5648:
URL: https://github.com/apache/nifi/pull/5648#issuecomment-1008139376
According the comment from @mattyb149 closing this PR and opening a new one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org