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

Proposed fixes for issue 81 #82

Closed
wants to merge 10 commits into from
Closed

Conversation

bitsofinfo
Copy link

So basically this boiled down to creating some new implementations of serialize/deserialize that use ObjectOutputStream as well as support being called w/ optional compression (which might be useful due to SQS message size limits).

The unit test still works and I added a new test for the transient referenced flagged TestObject with the special methods as described at http://docs.oracle.com/javase/7/docs/api/java/io/ObjectOutputStream.html to verify that if people want to send object messages w/ transient flagged fields AND they have these methods implemented, things will work as they expect.

Hopefully this fix can be put in, or some options to somehow enable the caller to decide which serialization engine to run that the project owner could discuss/implement? What do you think?

test for transient reference on serialization
added serialize/deserialize method versions that use ObjectOutputStream
for issue 81
for issue 81
for issue 81
for issue 81
@carterpage
Copy link
Member

Sorry for the long delay. This looks good at first glance. I'm going to take another more thorough review before I approve.

@bitsofinfo
Copy link
Author

Agreed cause I'm not sure I consistently implemented the calls to the OO versions, i.e. There might be paths that still call the hessian version.

My bigger question is why was the hessian one used originally and should we make the OO serialization an configurable option as to not break back compatibility. I imagine that there are some behaviors if the hessian library that existing users are knowingly or unknowingly dependent on.....

@carterpage
Copy link
Member

Did Hessian originally for size reasons, but a lot of users have complained
with this particular choice. I'd love to make the OO serialization
configurable. Please feel free to implement and submit a PR if you have
time. We'd need to make hessian the default but we should allow a user to
plug in anything they want.

On Thu, May 1, 2014 at 1:55 PM, bitsofinfo [email protected] wrote:

Agreed cause I'm not sure I consistently implemented the calls to the OO
versions, i.e. There might be paths that still call the hessian version.

My bigger question is why was the hessian one used originally and should
we make the OO serialization an configurable option as to not break back
compatibility. I imagine that there are some behaviors if the hessian
library that existing users are knowingly or unknowingly dependent on.....


Reply to this email directly or view it on GitHubhttps://github.com//pull/82#issuecomment-41907417
.

@carterpage
Copy link
Member

I'm going to do some work on this this weekend to allow you to define and plug-in custom serializers. I'll have to continue to default to hessian for backwards compatibility, but that should provide the hook users will need to change to anything they want.

@carterpage carterpage closed this May 22, 2014
@bitsofinfo
Copy link
Author

Sounds good, thanks! I figured back compat would be a concern so you will definately want to make it configurable. Post and update on #81 when you have something to test

@carterpage
Copy link
Member

Will do.

On Thu, May 22, 2014 at 3:37 PM, bitsofinfo [email protected]:

Sounds good, thanks! I figured back compat would be a concern so you will
definately want to make it configurable. Post and update on #81https://github.com/skyscreamer/nevado/issues/81when you have something to test


Reply to this email directly or view it on GitHubhttps://github.com//pull/82#issuecomment-43934339
.

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