[wip] CLOUDSTACK-8708: gson update and ArrayTypeAdaptor rewrite#932
Closed
DaanHoogland wants to merge 15 commits into
Closed
[wip] CLOUDSTACK-8708: gson update and ArrayTypeAdaptor rewrite#932DaanHoogland wants to merge 15 commits into
DaanHoogland wants to merge 15 commits into
Conversation
Contributor
There was a problem hiding this comment.
Our standard currently is to pin to a specific version, in this case you depend on any 2.4.X version. Although I understand your reasons, is this the PR to change that behavior?
It makes testing harder when deps change as long as we don't have proper testing in place.
Contributor
Author
There was a problem hiding this comment.
at the moment this list is a mess. not all deps are in it and the list is unsorted making it very hardd to maintain so I would prefer an automatic update like above.
29b0dda to
4cad5ae
Compare
Contributor
Author
|
So far so good, all test passed in CI. This touches every command and answer that is serialized, however. please all test |
Contributor
|
So, this PR is hard to test. For now I've verified that it compiles on my Ubuntu desktop with Java 8 and all the Unit Tests succeed, so that's good. Asking @borisroman if we can spin up a KVM cluster and verify the Agent communcation. This PR is a blocker for #625 since that PR also requires the new Gson. |
Contributor
|
@wido Will do! |
Member
There was a problem hiding this comment.
what do you think about using "return (Answer[])Array.newInstance(Answer.class, size);"?
Contributor
Author
There was a problem hiding this comment.
it is the same and the optimizer will treat it the same but this is easier to debug
Member
|
@ DaanHoogland, I used JDK 1.8.66, running from eclipse into a Windows 8 environment. |
Contributor
Author
|
Thanks @rafaelweingartner , i'll look at your bit and merge it in. |
Contributor
|
@DaanHoogland I tested this PR again today. 👎 When using the listSystemVms API call: And in general when collecting host stats: So it doesn't work in my setup. Could you have a look again? And also run the vm lifecycle integration tests and post the results, please. Ping @wido |
Contributor
|
@DaanHoogland Could you please have a look again? Adding primary storage doesn't function :( |
Contributor
|
@DaanHoogland I have the same result as @borisroman reports: |
Contributor
Author
|
This is far from done and i'll change the title to be prefixed with [wip] it is a crosscutting change the touches all internal communication of cloudstack. It is still at a 'what unit tests do we need to define?' state. Even if we pass all integration tests we can not be completely sure of this one, I'm afraid |
53098da to
7d13b1f
Compare
Contributor
Author
|
@remibergsma @borisroman @wido I just pushed a 650 file commit. I fair the future of this change. |
Contributor
|
Well... We could have expected it... https://sites.google.com/site/gson/gson-roadmap After version 2.0 Primtive Types weren't allowed anymore. If it works and is properly tested, then the future is bright :) |
2b1b556 to
8576aa7
Compare
8576aa7 to
bb155bb
Compare
Contributor
Author
|
get #987 working first then rethink this code. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
unit test for RequestTest is passing in eclipse. On macosx 10.11 using java 1.8 or 1.7 it fails from the command line.