Skip to content
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

enabling error reporting using supplyReporter #1348

Merged
merged 4 commits into from
Dec 26, 2024

Conversation

ankitk-me
Copy link
Contributor

No description provided.

Comment on lines 145 to 153
final Consumer<Throwable> report = exceptions
final Consumer<Throwable> reporter = exceptions
? e -> e.printStackTrace(System.err)
: e -> System.err.println(e.getMessage());

final ErrorHandler onError = ex ->
{
report.accept(ex);
reporter.accept(ex);
stop.countDown();
};
Copy link
Contributor

@jfallows jfallows Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still code duplication between zilla start here and the default reporter in EngineBuilder.

If we omit the reporter then it will default.

Perhaps it should default to reporting the exception and then triggering the error handler by default when onError would otherwise be called, so that we are not forced to explicitly create the reported here just to chain with the error handler.

We should also move the defaulting logic to EngineConfiguration. For that to work, we'll need to create an inner functional interface in EngineConfiguration, something like ErrorReporter which has void report(Throwable ex) or similar and default method that returns a different value depending on exceptions configuraiton setting in EngineConfiguration.

Note that the property type might need to be a Supplier<Consumer<Throwable>> instead of just Consumer<Throwable> so that it can return a distinct instance per EngineWorker.

Comment on lines 78 to 79
Consumer<Throwable> supplyReporter();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Consumer<Throwable> supplyReporter();
void report(
Throwable ex);

Perhaps this is sufficient, since the reporter would be created once per EngineWorker and reused for the same core anyway.

@@ -119,6 +124,10 @@ else if (keyTypes != null)
}
}
}
if (debug)
{
System.out.printf("No match found for Subject CN and Key Types");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the actual CN, key types and issuers here please.

{
props.setProperty(ENGINE_VERBOSE.name(), Boolean.toString(verbose));
}
props.setProperty(ENGINE_VERBOSE.name(), Boolean.toString(verbose));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change of behavior, because it will explicitly override verbose to false if not specified instead of picking up the value from zilla.properties or the -P value.

Please restore the if check and add a similar check for exceptions too.

@jfallows jfallows merged commit 72786eb into aklivity:develop Dec 26, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants