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

use Long insead of Integer in class GlanceImage #876

Merged
merged 4 commits into from
Feb 8, 2017

Conversation

whaon
Copy link
Contributor

@whaon whaon commented Nov 1, 2016

fix issue: #875

@whaon whaon closed this Nov 1, 2016
@whaon whaon reopened this Nov 1, 2016
@whaon
Copy link
Contributor Author

whaon commented Nov 2, 2016

@auhlig ,hi,I've submit serveral issues and a PR,please help me to check it,thanks

@auhlig
Copy link
Member

auhlig commented Nov 2, 2016

Hey @whaon, @tongh-qtl,
Thanks for the work.
I'm wondering if this is actually causing problems as the API specifically mentions integer for the properties you're referring to: http://developer.openstack.org/api-ref/image/v2/?expanded=show-image-details-detail#show-image-details

@whaon
Copy link
Contributor Author

whaon commented Nov 3, 2016

yes,it is definitely can cause a problem,consider the introduction of the field size:

size    body    integer The size of the image data, in bytes. The value might be null (JSON null data type).

the unit is in bytes,so the value may be two long
our system get a problem yesterday,so I think it is a potential problem to others
I think the integer in the doc is just indicate that this field is a nonnegative number,but the size may be huge,and it is not the concept of Interger in Java,thank you!
Long is always right but Integer not!

@auhlig
Copy link
Member

auhlig commented Nov 3, 2016

I don't have a strong opinion here. Besides my previous comment - maybe I misinterpreted integer in the docs - I would be fine with this change. What do you think @vinodborole?

@vinodborole
Copy link
Contributor

same thought @auhlig I am not sure, what do you think @gondor ?

@whaon
Copy link
Contributor Author

whaon commented Nov 4, 2016

the unit is in bytes

/**
     * A constant holding the maximum value an {@code int} can
     * have, 2<sup>31</sup>-1.
     */
    public static final int   MAX_VALUE = 0x7fffffff;

as above,the Integer.MAX_VALUE is 2147483647
2147483647bytes / 1024 / 1024 /1024 = 1.999999999068677425384521484375G
so the size of an Image can be as large as 2GB
but in actural,many Image can be more than 2GB

@vinodborole
Copy link
Contributor

@whaon the change is valid no doubt about that. Will need a final confirmation from @gondor on this as this will not be backward compatible, people using the older version will have an impact on their exiting code when they port to new version.

@whaon
Copy link
Contributor Author

whaon commented Nov 4, 2016

@vinodborole It's OK

@gondor
Copy link
Member

gondor commented Nov 4, 2016

@vinodborole @auhlig Should we consider cutting a new release first (soon) before this merge since there is a lot of changes already in the release. Then we could have a smaller quicker follow on release with a few fixes and changes. It would be easier to see the breaking changes this way. Thoughts?

@auhlig
Copy link
Member

auhlig commented Nov 4, 2016

@gondor Since murano service in os4j is unfortunately not making progress I guess it's the right time. Would leave current PRs open until after the release. So +1 for os4j 3.0.3

@whaon
Copy link
Contributor Author

whaon commented Nov 7, 2016

@auhlig @vinodborole @gondor
hi,may I close this PR and reopen it after the release 3.0.3,because there are another two issues need be fixed: #878 #877

@vinodborole
Copy link
Contributor

@gondor I completely agree!

@whaon
Copy link
Contributor Author

whaon commented Nov 11, 2016

close temporarily

@whaon whaon closed this Nov 11, 2016
1.compute:ContainX#17 getImageId()
2.image:#1 out of range of int,should use Long
3.image:#2 “result” in GlanceTask should be a Map
4.image:#3 imageStatus in GlanceImage should be status
@whaon
Copy link
Contributor Author

whaon commented Jan 9, 2017

how about this issue now?Guys

@vinodborole
Copy link
Contributor

@whaon will you be able to take this now?

@vinodborole vinodborole reopened this Feb 8, 2017
@vinodborole
Copy link
Contributor

reference to #875

@auhlig can you please review this pull request?

@auhlig
Copy link
Member

auhlig commented Feb 8, 2017

Good to merge from my side 👍

@auhlig auhlig added this to the 3.0.4 Release milestone Feb 8, 2017
@vinodborole vinodborole merged commit 6f970dc into ContainX:master Feb 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants