-
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
Add ServerErrorHandler #32
Conversation
Map<String, Object> data = new HashMap<String, Object>(); | ||
data.put("k3", "v3"); | ||
data.put("k4", "v4"); | ||
logger.log("test01", data); |
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.
Please add a comment here that says 'writing to the closed socket'
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.
Okay
@@ -132,4 +132,10 @@ public void finalize() { | |||
public boolean isConnected() { | |||
return sender != null && sender.isConnected(); | |||
} | |||
|
|||
public synchronized void setServerErrorHandler(ServerErrorHandler serverErrorHandler) { | |||
if (sender != null) { |
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.
Passing null handler is user error. Throw IllegalArgumentException here to notify the user that there is something wrong in the code.
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 thought the users can clear the error handler they set before if they pass null
. Should we create another API such as resetServerErrorHandler
?
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.
Sounds good. Please add removeXXXHandler (resetXXX sounds settings the handler again)
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 we need to set errorHandler
instance to DefaultErrorHandler
to avoid that the user fails to log an exception when (reset|remove)ErrorHandler is called. So reset sounds more appropriate to me.
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.
Reset to DefaultErrorHandler sounds good.
LGTM! |
No description provided.