-
Notifications
You must be signed in to change notification settings - Fork 91
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
Improving error handling #15
Comments
(Plan3) Catch all of the exception within |
(Plan4) Returns true/false and when false, users retrieve |
Thank you for your suggestions. Interesting. To customize fluent-logger behavior, users can extend to Sender class and implement it. But I think that your approach is better and more beautiful instead. Plan 2 looks simpler and better to me now. Plan 1 is also interesting. But it takes a long time for considering the design and APIs and needs trial and error. What do you think about his plans? |
IMO, Plan 2 looks much simplest way to implement and use. I believe that it can be implemented straightforwardly. Is the error handler interface like this?
|
I continued the discussion with @komamitsu in Twitter. In his idea, handling checked exception with try and catch clause is cumbersome for the users. I agree with him in this point. If we adopt Exception based API, backward compatibility to 0.2.x will be lost and force the users to wrap their code with try and catch. @oza Thanks. The default implementation of that interface would report error messages to a slf4j logger. To make the problem clear, I am going to present several code examples of reporting system information to fluentd: This part is common for all plans: FluentLogger logger = FluentLogger.getLogger(...); Plan1: Using ErrorHandler final AtomicBoolean toContinue = new AtomicBoolean(true);
logger.setErrorHandler(new DefaultErrorHandler { // Extending the default error handler
@Override
public void handleIOException(IOException e, Event lastEvent) {
toContinue.set(false);
}
});
while(toContinue.get()) {
// This can be a blocking or non-blocking method.
logger.log(tag, (system info data));
TimeUnit.SECONDS.sleep(1);
} Plan2: Checked exception boolean toContinue = true;
while(toContinue) {
try {
logger.log(tag, (system info data));
TimeUnit.SECONDES.sleep(1);
}
catch(IOException e) {
toContinue = false;
}
catch(OtherException e) {
....
}
} Plan3: Returns rich Success/Failure object boolean toContinue = true;
while(toContinue) {
LoggingResult rep = logger.log(tag, (system info data));
if(rep.isFailure) {
rep.cause(); // Retrieve exception
rep.lastLog(); // Retrieve last log
toContinue = false;
}
TimeUnit.SECONDES.sleep(1);
} Plan4: Returns true/false boolean toContinue = true;
while(toContinue) {
boolean success = logger.log(tag, (system info data));
if(!success) {
logger.lastError(); // Retrieve exception
logger.lastLog(); // Retrieve last log
toContinue = false;
}
TimeUnit.SECONDES.sleep(1);
} |
Wow, Plan1 (Using an error handler) makes the user code simplest. |
Plan2 looks simple. But as mentioned earlier, if we adopt checked exception, the users need to change their code. If adopting unchecked exception, the users' applications can be troubled because of the unexpected exceptions that haven't occurred until now. For plan3, actually I like this style (ideally speaking, I want to use pattern matching syntax...), but it changes the type of return value. For plan4, it looks simple and doesn't force the user change their code. But it returns false value if log() fails, not only when the buffer is full, right? @xerial Plan1 totally looks good to me. The casual users don't need to change their code because the default behavior is same as the current version. For those who want to know all errors that occurs in FluentLogger, I think it can satisfy their requirements. |
@komamitsu Right. if plan4 returns false, its cause can be buffer full, send error, message pack error, etc. |
+1 Plan1 |
Plan1++ |
I agree with you. Plan1 allows user to customize error handling easier. I think that it's good to design and implement the feature in the next major version. Next, to design error handling as @oza and @xerial said before, we should consider, design and implement exceptions (and errors) that fluent-logger can (and cannot) handle. |
@muga |
@xerial I just sent a PR for this issue. Can you look at it when you have a time? |
Adding error handler looks good. Commented to #32. Key points include:
|
@xerial Thanks for the comments. I added additional changes according to your comment. But I'm still wondering if we should prepare a default error handler since I have a concern about it #32 (comment). What do you think? |
I and @komamitsu san discussed how to improve error reporting of the current 0.2.x versions.
The major changes we considered are:
setHandler(handler)
method to FluentLogger to accept an application-specific error handler.In this change, we should consider the following problem:
log
method (true or false), unmanaged exceptions or exceptions thrown when the buffer is full. If an error handler is added,log
method should be non-blocking method, and the error must be handled in the user-defined error handler (in the subsequent code or in another thread). Does it the right choice?Another option would be:
log
a blocking method and reporting errors by Exception rather than returning true or false. The last log event should be included in the thrown exception.After writing this ticket, Plan 2 now looks simpler to me.
Any idea?
The text was updated successfully, but these errors were encountered: