-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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-11772 Removing flow.xml support #7661
Conversation
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.
Thanks for working through this extensive set of files @simonbence! With the removal of Templates now merged under NIFI-12006 can you rebase this PR and indicate when it is ready for review?
24db25d
to
d5ab505
Compare
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.
Thanks for working on this change @simonbence! I noted a few details to address.
In particular, commented out methods need to be addressed, either corrected or removed. In the case of the encrypt-config command, it should already support flow.json.gz, but methods need to be renamed.
As far as the framework integration tests in nifi-framework-cluster, I believe it would be better to just remove all of them since they are not actively maintained or run. Perhaps @markap14 can weigh in on this question, but the system-test-suite should provide all the necessary coverage, and those older tests, along with accompanying flow configurations, can be removed.
The removal of the legacy code related to port authorization looks good.
One general question is whether we should switch the configuration property or keep the json.file property. Relying on the json.file property name would simplify upgrading without having to change property values. On the other hand, it is a simple manual change that seems reasonable. Thoughts?
...mons/nifi-flow-encryptor/src/main/java/org/apache/nifi/flow/encryptor/JsonFlowEncryptor.java
Outdated
Show resolved
Hide resolved
...framework-bundle/nifi-framework/nifi-framework-core/src/main/resources/FlowConfiguration.xsd
Outdated
Show resolved
Hide resolved
...-framework-bundle/nifi-framework/nifi-framework-core/src/test/resources/conf/nifi.properties
Outdated
Show resolved
Hide resolved
...amework/nifi-properties-loader/src/test/resources/properties/conf/duplicates.nifi.properties
Show resolved
Hide resolved
...-test-suite/src/test/java/org/apache/nifi/tests/system/clustering/FlowSynchronizationIT.java
Outdated
Show resolved
Hide resolved
...uite/src/test/java/org/apache/nifi/tests/system/clustering/JoinClusterWithDifferentFlow.java
Outdated
Show resolved
Hide resolved
assertFalse(procDto.getId().endsWith("00")); | ||
assertFalse(procDto.getName().endsWith("00")); | ||
} | ||
// TODO we need a JSON analogue for this |
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.
We should remove this method and create a new Jira issue instead of commenting out the method.
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.
I fixed the test within the story. Please take a look. There are some things you might want to ask about, especially the removeal of the disconnection. As far as I understood it 1. did not work as expected even before the PR 2. was unnecessary for the given test scenario. I might miss something so please mention if you disagree with any of the changes
...oolkit-encrypt-config/src/main/groovy/org/apache/nifi/properties/ConfigEncryptionTool.groovy
Outdated
Show resolved
Hide resolved
...oolkit-encrypt-config/src/main/groovy/org/apache/nifi/properties/ConfigEncryptionTool.groovy
Outdated
Show resolved
Hide resolved
Thanks for thoroughly reviewing @exceptionfactory . I'll plan on reviewing this in more depth next week as well. In the meantime, I do agree with your comment above re: deleting the antiquated integration tests. |
Thanks for the confirmation on the integration tests @markap14! |
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.
@simonbence I removed the obsolete framework integration tests in #7784, so if you can rebase, that would streamline the review.
d5ab505
to
d642f00
Compare
Hi @exceptionfactory ! Thank you very much for the effort you did put on the review! I added some changes. Also answering your question:
|
d642f00
to
e449fb6
Compare
@@ -93,14 +91,8 @@ public Port updatePortAdvice(ProceedingJoinPoint proceedingJoinPoint, PortDTO po | |||
final int maxConcurrentTasks = port.getMaxConcurrentTasks(); | |||
|
|||
final Set<String> existingUsers = new HashSet<>(); | |||
final Set<String> existingGroups = new HashSet<>(); | |||
boolean isPublicPort = false; |
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.
It looks like isPublicPort
will always be false here, as you've removed the if (port instanceof PublicPort) {... }
. I think we need to instead set boolean isPublicPort = port instanceof PublicPort;
while (getNifiClient().getControllerClient().getNodes().getCluster().getNodes().stream().filter(n -> n.getStatus().equals("CONNECTED")).count() != 2) { | ||
Thread.sleep(3000); | ||
} |
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.
This should all be replaced with waitForAllNodesConnected();
|
||
final List<File> flowXmlFiles = getFlowJsonFiles(confDir); |
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.
Variable name should have been flowJsonFiles
disconnectNode(node2Dto); | ||
verifyInMemoryFlowContents(); | ||
|
||
// Reconnect the node so that we can properly shutdown | ||
reconnectNode(node2Dto); |
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.
Is there a reason that we're changing the logic here?
Previously, the idea was to disconnect Node 2 so that any request made to the node would be a response directly from that node rather than a cluster response; then issue the request to Node 2 and verify that its flow is correct. Then, it would reconnect the node so that the shutdown logic would complete successfully.
The logic here has now changed to "verify that the cluster's flow is correct", which is not the same as "verify that Node 2's flow is correct."
That said, there is now a better way to handle this, by doing something like:
switchClientToNode(2);
getNifiClient().getFlowClient(DO_NOT_REPLICATE)...
This way, we don't have to disconnect the node, but we can issue requests directly to that node. But I think we need to either restore the logic or use the DO_NOT_REPLICATE to ensure that we're verifying Node 2's flow.
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.
Thanks! I would believe the previous logic did not work properly as it was before remoeval (it did a REST call against the endpoint cluster/nodes/${nodeId}
which behaves based on the accompanying NodeEntity
/ NodeDTO
, but the status was not set to be DISCONNECTING, so in practice I think it did not disconnect), but thanks for the context! I will look how it behaves this way
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.
Thanks for all the work you've put into this @simonbence. Not gonna lie, I was pretty scared when I saw 11,000 lines removed! But most of it was flow.xml files, etc. Made me feel much better :)
Overall I think it looks good. I commented inline a couple of concerns, but I think all are trivial to address. Otherwise, I'm a +1.
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.
Thanks for making the updates @simonbence. This looks closer to completion, pending responses to feedback from @markap14. I also noted a couple more changes necessary in the toolkit class.
...oolkit-encrypt-config/src/main/groovy/org/apache/nifi/properties/ConfigEncryptionTool.groovy
Outdated
Show resolved
Hide resolved
...oolkit-encrypt-config/src/main/groovy/org/apache/nifi/properties/ConfigEncryptionTool.groovy
Outdated
Show resolved
Hide resolved
...oolkit-encrypt-config/src/main/groovy/org/apache/nifi/properties/ConfigEncryptionTool.groovy
Show resolved
Hide resolved
return new PropertyEncryptorBuilder(propertiesKey).setAlgorithm(propertiesAlgorithm).build(); | ||
final ControllerServicesEntity controllerLevelServices = getNifiClient().getFlowClient(DO_NOT_REPLICATE).getControllerServices(); | ||
final Set<ControllerServiceEntity> controllerServices = controllerLevelServices.getControllerServices(); | ||
assertEquals(2, controllerServices.size()); |
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.
I had some troubles with this assertation as it did persistently behaved differently as in case of the XML based synchronization. I looked into the implementation and I found that, the JSON based synchronization behaves differently for the flow itself and the controller/management level parts of the flow definition. While the items part of the flow (root group) might be "replaced" (added and removed) when id is changed, and that part of the test still covers the behaviour correctly, the methods like inheritControllerServices
within VersionedFlowSynchronizer
seemingly work differently and does not remove items not appearant in the proposed flow. This is why in case of JSON sync, the resuolt contains the controller service from both flow1 and flow2. @markap14 could you please confirm this behaviour from the part of the VersionedFlowSynchronizer
is deliberate? Thanks! (Also, thank you for highlighting that the original change sometimes checked on the flow of node 1)
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.
So you're saying that when using the XML form, the final result was 1 Controller Service. With the JSON form, it has two Controller Services, because it didn't remove the service from the first flow?
If that is so, I would say that it's a bug. So I'd say that it would make sense to:
- Have the system test assert that the size is equal to 1.
- Use @disabled on the system test because we know it's not working
- File a Jira to fix the issue and re-enable the system test.
This way, we can continue on and get this PR merged and avoid tackling other (potentially complex) bugs.
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.
I think that is correct. The original test had assertion to check for 1 Controller Service. As I mentioned there are a lot of moving parts (prod code, test code, test data) but based on what I did see, there is a behaviour in difference compared to the XML. I created a ticket for this: https://issues.apache.org/jira/browse/NIFI-12203
I will push a commit soon in order to adjust the test.
// Verify some of the values that were persisted to disk | ||
final File confDir = backupFile.getParentFile(); | ||
final String loadedFlow = readFlow(new File(confDir, "flow.xml.gz")); | ||
final String loadedFlow = readFlow(new File(confDir, "flow.json.gz")); |
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.
By some reason there is some misbehaviour with the id handling of the registry clients. At the time of implementation there was some necessary duplication of it, as we needed to support both the legacy solution (getId) and the solution derived from the component handling (getIdentifier). It looks like, in case of synchronization this might cause some issues as id is used for both instanceIdentifier and Identifier. That can couse the following error if we have different registry clients in the synchronized flows:
Proposed Flow is not inheritable by the flow controller because of differences in missing components: Current flow has missing components that are not considered missing in the proposed flow (65b71b80-016e-1000-7131-40b0976ac45966)
This is something was discovered within the effors led to this PR but not part of the scope or direct result of the changes. I will raise a separate apache ticket and PR for a fix on this.
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.
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.
Thanks for working through the feedback @simonbence, the latest version appears to address the remaining feedback.
I pushed one more commit to remove the FlowConfiguration.xsd, which is no longer needed as noted earlier.
Do you have any additional feedback @markap14? If not, I can proceed with merging soon.
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.
Confirmed the latest changes with @markap14 in offline conversation. Thanks again for the work on this @simonbence! +1 merging to main!
This is a draft pull request in order to finalize the approach. Things not aimed in this PR:
Summary
NIFI-11772
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000
NIFI-00000
Pull Request Formatting
main
branchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-check
Licensing
LICENSE
andNOTICE
filesDocumentation