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/02/22 18:53:10 UTC
[GitHub] [nifi] greyp9 opened a new pull request #5787: NIFI-9233 - Improve reliability of system integration tests
greyp9 opened a new pull request #5787:
URL: https://github.com/apache/nifi/pull/5787
#### Description of PR
Various code fixes related to Github/CI system test failures.
- Capture troubleshooting data on job timeout.
- Capture `maven-failsafe-plugin` output on job failure.
- System test timing adjustments to reduce diagnostic noise.
- Log PID on startup / shutdown to identify source of orphaned processes.
- Fix for `LoadBalanceSession` handshake failure (which causes test timeouts).
- Fix for `LoadBalanceSession` session wrapup (which can cause test timeouts).
- Logging allowing for identification of load balance failures.
- Retry NiFi process shutdown if needed during system test case teardown.
- Fix logging typo.
- Log IDs of both ends of LoadBalance connections (to help identify failure cases).
- Retry ReportingTask query on incomplete response.
- Do not mask `InterruptedException` while waiting for system test actions to complete.
- Retry service queries until expected state is verified.
- Short waits between actions (when needed) to allow FlowManager state to become consistent.
- Comment out ReportingTask system test failures to allow NIFI-9717 to be addressed asynchronously.
- Adjust system test cluster configuration to avoid test failure condition.
- Adjust `logback` logging for system test profile (include timestamps).
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:
- [X] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [X] Have you written or updated unit tests to verify your changes?
- [X] Have you verified that the full build is successful on JDK 8?
- [X] 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] exceptionfactory commented on a change in pull request #5787: NIFI-9233 - Improve reliability of system integration tests
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5787:
URL: https://github.com/apache/nifi/pull/5787#discussion_r814831662
##########
File path: nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/verification/ClusteredVerifiableControllerServiceSystemIT.java
##########
@@ -57,6 +57,6 @@ public void testDifferentResultsFromDifferentNodes() throws InterruptedException
// Second verification result will be verification results
assertEquals(Outcome.SUCCESSFUL.name(), resultList.get(1).getOutcome());
// Third verification result is for Fail On Primary Node
- assertEquals(Outcome.FAILED.name(), resultList.get(2).getOutcome());
+ // assertEquals(Outcome.FAILED.name(), resultList.get(2).getOutcome()); // NIFI-9717
Review comment:
Thanks for the response @greyp9, that is helpful. I have seen a number of ignored tests here and there, so that was the initial reason for flagging the commented lines. Given that you already created a specific Jira issue and mentioned it in the comment, it seems acceptable to leave the commented line for now. Hopefully that issue can be addressed soon after resolving the general system test stability issues.
--
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] greyp9 commented on a change in pull request #5787: NIFI-9233 - Improve reliability of system integration tests
Posted by GitBox <gi...@apache.org>.
greyp9 commented on a change in pull request #5787:
URL: https://github.com/apache/nifi/pull/5787#discussion_r814825568
##########
File path: nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/verification/ClusteredVerifiableControllerServiceSystemIT.java
##########
@@ -57,6 +57,6 @@ public void testDifferentResultsFromDifferentNodes() throws InterruptedException
// Second verification result will be verification results
assertEquals(Outcome.SUCCESSFUL.name(), resultList.get(1).getOutcome());
// Third verification result is for Fail On Primary Node
- assertEquals(Outcome.FAILED.name(), resultList.get(2).getOutcome());
+ // assertEquals(Outcome.FAILED.name(), resultList.get(2).getOutcome()); // NIFI-9717
Review comment:
My overarching goal has been to get the system tests green. Commenting out the assertion and removal are two strategies that both work toward this goal.
Another aspect of this is that this assertion passes upwards of 95% of the time. So when the issue is resolved, we would want to reinstate the assertion. My opinion is the "lift" required to do that is slightly easier using the "comment out" strategy.
There is some interesting discussion here:
- https://stackoverflow.com/questions/2501519/delete-or-comment-out-non-working-junit-tests
--
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] greyp9 commented on a change in pull request #5787: NIFI-9233 - Improve reliability of system integration tests
Posted by GitBox <gi...@apache.org>.
greyp9 commented on a change in pull request #5787:
URL: https://github.com/apache/nifi/pull/5787#discussion_r814817114
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/queue/clustered/server/ConnectionLoadBalanceServer.java
##########
@@ -169,8 +169,10 @@ public void run() {
break;
}
} catch (final Exception e) {
+ stopped = true;
if (socket != null) {
try {
+ logger.debug("closing socket");
Review comment:
removed (leftover debugging)
--
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] greyp9 closed pull request #5787: NIFI-9233 - Improve reliability of system integration tests
Posted by GitBox <gi...@apache.org>.
greyp9 closed pull request #5787:
URL: https://github.com/apache/nifi/pull/5787
--
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] greyp9 commented on pull request #5787: NIFI-9233 - Improve reliability of system integration tests
Posted by GitBox <gi...@apache.org>.
greyp9 commented on pull request #5787:
URL: https://github.com/apache/nifi/pull/5787#issuecomment-1057507510
> @greyp9, in order to continue progress on this pull request, what do you think about separating the `nifi-bootstrap` changes into a separate PR? Including the Process ID looks like a useful runtime adjustment, regardless of system integration tests. Those changes could be reviewed and incorporated separately from the integration test improvements.
Thanks @exceptionfactory for the feedback. Looking at this PR as a whole, I agree that it should be broken out, and I'm in the process of doing that. I'll close this PR in favor of those.
> The runtime change to `LoadBalanceSession` also seems worth considering separately, but in that case, I'm not sure that hard-coding a one second read timeout makes sense in all cases.
Yep. The one second timeout only affects the server ACK part of LoadBalance protocol. I tried it out in response to direct evidence of an issue with that part of the protocol, and things ran much better on the github CI machines. But I agree it looks icky. Something is going on there that needs further investigation.
--
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] greyp9 commented on pull request #5787: NIFI-9233 - Improve reliability of system integration tests
Posted by GitBox <gi...@apache.org>.
greyp9 commented on pull request #5787:
URL: https://github.com/apache/nifi/pull/5787#issuecomment-1057528047
Closing this PR; many of these changes are included in PRs: 5826, 5827, 5828, 5829.
--
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 #5787: NIFI-9233 - Improve reliability of system integration tests
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5787:
URL: https://github.com/apache/nifi/pull/5787#discussion_r817704940
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/ShutdownHook.java
##########
@@ -66,7 +68,7 @@ public void run() {
}
runner.notifyStop();
- System.out.println("Waiting for Apache NiFi to finish shutting down...");
+ System.out.println("Waiting for Apache NiFi to finish shutting down... pid=" + pid);
Review comment:
Also recommend adjusting this message:
```suggestion
System.out.printf("NiFi process [%d] shutdown in progress...%n", pid);
```
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/ShutdownHook.java
##########
@@ -51,7 +53,7 @@ public void run() {
runner.setAutoRestartNiFi(false);
final int ccPort = runner.getNiFiCommandControlPort();
if (ccPort > 0) {
- System.out.println("Initiating Shutdown of NiFi...");
+ System.out.println("Initiating Shutdown of NiFi... pid=" + pid);
Review comment:
Recommend the following in relationship to the comment on `RunNiFi`, as well as adjusting the method call to use `printf()` for formatting:
```suggestion
System.out.printf("NiFi process [%d] shutdown started%n", pid);
```
--
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 #5787: NIFI-9233 - Improve reliability of system integration tests
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5787:
URL: https://github.com/apache/nifi/pull/5787#discussion_r814465728
##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/queue/clustered/server/ConnectionLoadBalanceServer.java
##########
@@ -169,8 +169,10 @@ public void run() {
break;
}
} catch (final Exception e) {
+ stopped = true;
if (socket != null) {
try {
+ logger.debug("closing socket");
Review comment:
This logging statement should include more details about the connection.
##########
File path: nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/verification/ClusteredVerifiableControllerServiceSystemIT.java
##########
@@ -57,6 +57,6 @@ public void testDifferentResultsFromDifferentNodes() throws InterruptedException
// Second verification result will be verification results
assertEquals(Outcome.SUCCESSFUL.name(), resultList.get(1).getOutcome());
// Third verification result is for Fail On Primary Node
- assertEquals(Outcome.FAILED.name(), resultList.get(2).getOutcome());
+ // assertEquals(Outcome.FAILED.name(), resultList.get(2).getOutcome()); // NIFI-9717
Review comment:
Rather than commenting out this assertion, it seems like it should be removed, then evaluated in the ticket mentioned.
--
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 #5787: NIFI-9233 - Improve reliability of system integration tests
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5787:
URL: https://github.com/apache/nifi/pull/5787#discussion_r817698859
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/ShutdownHook.java
##########
@@ -51,7 +53,7 @@ public void run() {
runner.setAutoRestartNiFi(false);
final int ccPort = runner.getNiFiCommandControlPort();
if (ccPort > 0) {
- System.out.println("Initiating Shutdown of NiFi...");
+ System.out.println("Initiating Shutdown of NiFi... pid=" + pid);
Review comment:
Recommend the following in relationship to the comment on `RunNiFi`, as well as adjusting the method call to use `printf()` for formatting:
```suggestion
System.out.printf("NiFi process [%s] shutdown started%n", pid);
```
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -116,7 +116,7 @@
public static final String PID_KEY = "pid";
public static final int STARTUP_WAIT_SECONDS = 60;
- public static final long GRACEFUL_SHUTDOWN_RETRY_MILLIS = 2000L;
+ public static final long GRACEFUL_SHUTDOWN_RETRY_MILLIS = 1000L;
Review comment:
Is there a particular reason for reducing retry interval?
##########
File path: nifi-bootstrap/src/main/java/org/apache/nifi/bootstrap/RunNiFi.java
##########
@@ -1056,7 +1056,7 @@ private void waitForShutdown(final String pid, final Logger logger, final File s
logger.error("Failed to delete pid file {}; this file should be cleaned up manually", pidFile);
}
- logger.info("NiFi has finished shutting down.");
+ logger.info("NiFi has finished shutting down. (PID={})", pid);
Review comment:
References to the Process ID appear differently in this class versus `ShutdownHook`. Recommend keeping `PID` capitalized and adjusting the log as follows:
```suggestion
logger.info("NiFi process [{}] shutdown completed", pid);
```
--
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