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.

Sign up for GitHub

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

Jump to bottom

Make Protostream the default marshaller for user types #7302

Closed
wants to merge 7 commits into from
Closed

Make Protostream the default marshaller for user types #7302

wants to merge 7 commits into from

Conversation

ryanemerson
Copy link
Contributor

@ryanemerson ryanemerson commented Sep 12, 2019 •
edited

https://issues.jboss.org/browse/ISPN-10591

Builds upon the commits in #7264, which should be merged first.

@ryanemerson ryanemerson force-pushed the ISPN-10591 branch 3 times, most recently from 38db4b7 to eb6fdcc Compare September 13, 2019 13:39
@ryanemerson ryanemerson force-pushed the ISPN-10591 branch 4 times, most recently from 9b7a6d4 to dfbd6ed Compare September 16, 2019 10:19
@ryanemerson ryanemerson mentioned this pull request Sep 17, 2019
Closed
@diegolovison
Copy link
Contributor

run custom performance tests please
ispn_config https://gist.githubusercontent.com/ryanemerson/40e40efb6e73c8c19c8a5d42c3dfe32f/raw/99ded6d69066168d604705dccf9128cc564b63e2/PR-7153.xml
jdg_config https://gist.githubusercontent.com/ryanemerson/df5fd0c76241b9ced6b7d8ea92e1d924/raw/8bc5fe883060fd0a4d674e41fcf606f83e86f5cd/PR-7153-DG.xml
custom_library
entry_factor 8

@infinispan infinispan deleted a comment Sep 18, 2019
@ghost
Copy link

ghost commented Sep 18, 2019

Performance tests didn't finish successfully. @diegolovison, can you review it?

Additional info:
Commit: d757551
Build number: #105
Comment body: run custom performance tests please\r\nispn_config https://gist.githubusercontent.com/ryanemerson/40e40efb6e73c8c19c8a5d42c3dfe32f/raw/99ded6d69066168d604705dccf9128cc564b63e2/PR-7153.xml\r\njdg_config https://gist.githubusercontent.com/ryanemerson/df5fd0c76241b9ced6b7d8ea92e1d924/raw/8bc5fe883060fd0a4d674e41fcf606f83e86f5cd/PR-7153-DG.xml\r\ncustom_library\r\nentry_factor 8
skip ci

@anistor
Copy link
Member

anistor commented Sep 23, 2019

Protostream 4.3.0.Alpha12 is now released! ProtoStreamTypeIds.WRAPPED_MESSAGE can now be an alias of the newly introduced WrappedMessage.PROTOBUF_TYPE_ID constant to avoid a magic number :)

@ryanemerson ryanemerson force-pushed the ISPN-10591 branch 2 times, most recently from 907b017 to 69b7163 Compare September 23, 2019 13:00
@ryanemerson
Copy link
Contributor Author

On hold as I will remove the notion of the user marshaller in this PR.

@anistor
Copy link
Member

anistor commented Sep 23, 2019

On hold as I will remove the notion of the user marshaller in this PR.
Does that mean I won't be able to use java serialization or jboss marshalling to my heart's desire? I hope I'm not force to use protobuf only.

@ryanemerson
Copy link
Contributor Author

@anistor See the discussion on #marshalling and this week's meeting for more details. But in summary, the idea is that if you want those things for embedded mode then you should use AdvancedCache::withEncoding

@ryanemerson ryanemerson force-pushed the ISPN-10591 branch 2 times, most recently from dc07713 to eba2281 Compare September 23, 2019 17:57
@ryanemerson ryanemerson force-pushed the ISPN-10591 branch 2 times, most recently from b5a6a67 to 7247eaa Compare September 23, 2019 19:55
@anistor
Copy link
Member

anistor commented Sep 24, 2019

@anistor See the discussion on #marshalling and this week's meeting for more details. But in summary, the idea is that if you want those things for embedded mode then you should use AdvancedCache::withEncoding

My concern is about actually storing in cache java objects as objects, without using any protobuf at all (for reasons related to hibernate search indexing). Does withEncoding offer that or just sugar coats a byte[]? I'm not familiar with the latest status in this area.

@ryanemerson
Copy link
Contributor Author

@anistor The ability to configure a user marshaller is in place again, so this PR is good to go from my POV.

@ryanemerson ryanemerson added this to the 10.0.0.CR3 milestone Sep 25, 2019
@ryanemerson ryanemerson force-pushed the ISPN-10591 branch 4 times, most recently from ceb0dc3 to 5506702 Compare September 27, 2019 10:50
@anistor
Copy link
Member

anistor commented Oct 1, 2019

I've added some comments, that are very easy fixes. I'm happy with it and it LVVVGTM :)

@ryanemerson
Copy link
Contributor Author

Updated, thanks for the review @anistor

@anistor
Copy link
Member

anistor commented Oct 1, 2019

LGTM. Let's wait for another round of CI.

@ryanemerson
Copy link
Contributor Author

@anistor CI is good 🙂

@anistor
Copy link
Member

anistor commented Oct 2, 2019

merged. thanks @ryanemerson !

@anistor anistor closed this Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anistor anistor anistor left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
10.0.0.CR3
3 participants
@ryanemerson @diegolovison @anistor