-
Notifications
You must be signed in to change notification settings - Fork 53
BROOKLYN-451: Adds yaml-dsl rebind tests (for historic state) #593
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
0dd7132 to
c8b5e13
Compare
| // See ObjectWithDefaultStringImplConverter (and its usage) for why we want to auto-detect | ||
| // annotations (usages of this is in the camp project, so we can't just list it statically | ||
| // here unfortunately). | ||
| xstream.autodetectAnnotations(true); |
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.
What are the performance implications of this?!
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.
From http://x-stream.github.io/annotations-tutorial.html#AutoDetect
In auto-detection mode XStream will have to examine any unknown class type for annotations. This will slow down the marshalling process until all processed types have been examined once.
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.
Seems to be about 5-10% overhead (when testing with a simple EntityMemento that has lots of string keys and brooklyn.parameters). That's certainly acceptable, to fix the issue!
|
|
||
| @Override | ||
| public boolean canConvert(@SuppressWarnings("rawtypes") Class type) { | ||
| return true; |
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.
Worth adding a comment here as to why this is hardcoded
|
LGTM |
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.
LGTM
Tested successful rebind with existing state from 0.10.0 that was previously broken
c8b5e13 to
a536cdc
Compare
|
Thanks @neykov @m4rkmckenna. I've added the comment you suggested. Merging now. |
Tests + fixes https://issues.apache.org/jira/browse/BROOKLYN-451