-
Notifications
You must be signed in to change notification settings - Fork 2.9k
NIFI-15180 Log initialization errors in ConsumeKinesis #10526
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-15180 Log initialization errors in ConsumeKinesis #10526
Conversation
b6d9fc7 to
f2955c9
Compare
|
|
||
| final ProcessException ex = failure.error() | ||
| .map(err -> new ProcessException("Failed to initialize the processor.", err)) | ||
| .orElseGet(() -> new ProcessException("Failed to initialize the processor due to an unknown failure. Check application logs for more details.")); |
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 branch is active only when a scheduler was shutdown, but no initialization error was provided. However, I didn't observe this behavior while testing.
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.
If that is the case, it seems worth mentioning. Mentioning "check the application logs for more details" is not helpful because it already appears in the logs, so the message should be shortened.
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.
The reason behind "check the application logs for more details" is that the error text is visible in a bulletin.
When an initialization error is present, it's logged and, therefore available in the bulletin too.
When error is not available for some reason, the bulletin will just mention that initialization failed. However, additional details may be present in application logs, which aren't available in NiFi canvas.
Is it expected for the NiFi users to inspect application logs as well? If so, then the "check the application logs for more details" part is not needed.
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.
Bulletin messages are limited in nature, and often require checking logs as a general practice. More recent adjustments have introduced stack trace visibility for Bulletins, but checking the logs is almost always necessary, so including the statement in a message is redundant.
f2955c9 to
d6cabe6
Compare
exceptionfactory
left a comment
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 adjusting the approach and implementing waiting on successful initialization @awelless. This looks like a good way forward in general. I recommended a few adjustments.
...le/nifi-aws-kinesis/src/main/java/org/apache/nifi/processors/aws/kinesis/ConsumeKinesis.java
Outdated
Show resolved
Hide resolved
|
|
||
| final ProcessException ex = failure.error() | ||
| .map(err -> new ProcessException("Failed to initialize the processor.", err)) | ||
| .orElseGet(() -> new ProcessException("Failed to initialize the processor due to an unknown failure. Check application logs for more details.")); |
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.
If that is the case, it seems worth mentioning. Mentioning "check the application logs for more details" is not helpful because it already appears in the logs, so the message should be shortened.
...le/nifi-aws-kinesis/src/main/java/org/apache/nifi/processors/aws/kinesis/ConsumeKinesis.java
Outdated
Show resolved
Hide resolved
...le/nifi-aws-kinesis/src/main/java/org/apache/nifi/processors/aws/kinesis/ConsumeKinesis.java
Show resolved
Hide resolved
...ifi-aws-kinesis/src/test/java/org/apache/nifi/processors/aws/kinesis/ConsumeKinesisTest.java
Outdated
Show resolved
Hide resolved
...ifi-aws-kinesis/src/test/java/org/apache/nifi/processors/aws/kinesis/ConsumeKinesisTest.java
Outdated
Show resolved
Hide resolved
exceptionfactory
left a comment
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 the responses, this looks close to completion, I noted a few more minor recommendations.
...le/nifi-aws-kinesis/src/main/java/org/apache/nifi/processors/aws/kinesis/ConsumeKinesis.java
Outdated
Show resolved
Hide resolved
...le/nifi-aws-kinesis/src/main/java/org/apache/nifi/processors/aws/kinesis/ConsumeKinesis.java
Outdated
Show resolved
Hide resolved
exceptionfactory
left a comment
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 @awelless, the latest version looks good. +1 merging
Summary
NIFI-15180
So far the issues which failed Scheduler initialization are caused by misconfiguration and are not recoverable.
Like inability to access a Dynamodb/Kinesis endpoint due to invalid network rules, or an inability to act on a table/stream due to absent iam permissions.
If KCL scheduler fails to initialize, the processor's initialization fails as well.
The change was tested manually.
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-15180NIFI-15180Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation