-
Notifications
You must be signed in to change notification settings - Fork 19
remove Callable execution during msgpack serialization #26
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
Conversation
@@ -72,13 +72,6 @@ | |||
|
|||
public void pack(Object item, OutputStream os) throws IOException { | |||
DataOutputStream out = new DataOutputStream(os); | |||
if (item instanceof Callable) { |
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.
Remove without workaround? Also your could check the docs here https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Callable.html
The Callable interface is similar to Runnable, in that both are designed for classes whose instances are POTENTIALLY executed by another thread.
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.
Workaround is pretty clear - first execute Callable and then invoke MsgPackLite.pack()
on the result.
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 is not workaround for asynchronous case.
How this code could disturb you?
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.
There is no asynchronous case in the code I suggest to remove and thus no asynchronous workaround is needed.
This code violates single responsibility principle (mixing serialization and execution) and hence the disturbance.
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.
In some cases, you could do more with this ability.
You could simple not use this opportunity.
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.
U could see that enum Key use Callable interface to provide id. Did u test code after PR?
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.
OK, got it - 2962aa8 here are the tests
Callable are intended for executing something in separate thread(s) - simply executing them in the same thread during serialization is really misleading