[OSJC-101] Add support for setting and retrieving additional gear storag... #148
Conversation
There was a problem hiding this comment.
I think that we're missing #equals and #hasCode here. Especially having #equals return true if you have it compared to a standalone cartridge spec would be nice (like it's done in EmbeddableCartridge and EmbeddedCartridge)
jcantrill
force-pushed
the
osjc_101_support_gear_storage
branch
from
b13e4a1 to
8c3f348
Compare
|
@adietish Please review again. I pushed changes as advised and added an integration test. Trying to run them now to see if we can validate it before merge. |
jcantrill
force-pushed
the
osjc_101_support_gear_storage
branch
2 times, most recently
from
79436bf to
1a9b164
Compare
There was a problem hiding this comment.
why choose a somehow exotic naming scheme ("update") and not use the common "setXX" naming scheme?
There was a problem hiding this comment.
I only see StandaloneCartridgeResource and EmbeddedCartridgeResource to request additional storage, the other cartridge classes simply store the value (EmbeddableCartridge, StandaloneCartridge). IMHO we should then not add #updateAdditionalGearStorage to ICartridge but have this only in specific interfaces (IEmbeddedCartridge and IDeployedStandaloneCartridge (?)) since there's only added value for the deployed variants (StandaloneCartridgeResource and EmbeddedCartridgeResource) and no logic/added value for the cartridge definition classes (StandaloneCartridge, EmbeddableCartridge)
There was a problem hiding this comment.
@adietish I chose this name explicitly because of the information in the API documentation which is:
'additional_gear_storage Set additional filesystem storage in gigabytes for the gear profile that the cartridge is running on.' I confess I don't remember, if I tested it to see if it is truly a 'set' operation which would make sense given I think you would want to be able to lower or raise it depending upon your needs.
There was a problem hiding this comment.
@adietish In regards to where this interface method should reside, if you believe it should move to a separate interface I could be convinced to change it. Maybe this is an opportunity to define a capability for cartridges. Could it be a 'deployed cartridge'? Is there any significance to identifying it as 'embedded' or 'standalone'? I also havent looked, but does that also require us to cast when we use it?
There was a problem hiding this comment.
Hmm, I dont think that we should introduce major new design elements in the v2 library. I would simply make sure we have this in the 2 interfaces where we need it: the interface for StandaloneCartridgeResource (IDeployedStandaloneCartridge?) and IEmbeddedCartridge
jcantrill
force-pushed
the
osjc_101_support_gear_storage
branch
from
1a9b164 to
b30a223
Compare
|
thanks, looks much better now! |
There was a problem hiding this comment.
what looks weird to me is that we set the additional gear storage in the standalone cartridge while we can only get it in the gear group. Furthermore it looks to me as if you set the additional gear storage and then go and get the value from the gear group you'd get the old value?
I'm taking care of this.
There was a problem hiding this comment.
The odd ways of setting and getting are in line with what the REST API
provides. Your comment with regards to the value mismatch is probably
correct. We could correct this by either forcing a refresh or figuring
out how to update the gear group when setting the value on the Cartridge.
On 10/14/2014 01:33 PM, André Dietisheim wrote:
In src/main/java/com/openshift/client/cartridge/IEmbeddedCartridge.java:
@@ -52,4 +42,13 @@
* @return the resource properties
*/
public CartridgeResourceProperties getProperties();
- /**
\* set the additional gear storage for the cartridge to the given\* size.*\* @param size The total additional gear storage for the cartridge\* in gigabytes*/- public void setAdditionalGearStorage(int size);
what looks weird to me is that we set the additional gear storage in the
standalone cartridge while we can only get it in the gear group.
Furthermore it looks to me as if you set the additional gear storage and
then go and get the value from the gear group you'd get the old value?—
Reply to this email directly or view it on GitHub
https://github.com/openshift/openshift-java-client/pull/148/files#r18844358.
Jeff Cantrill
Senior Software Engineer, Red Hat Engineering
Red Hat
Office: 703-748-4420 | 866-546-8970 ext. 8162420
jcantril@redhat.com
http://www.redhat.com
There was a problem hiding this comment.
I added a IDeployedStandaloneCartridge#getAdditionalGearStorage and delegated to ApplicationResource behind the scenes: https://github.com/openshift/openshift-java-client/pull/167/files#diff-7f982b00d649ba3eb211bedaee0e4986R68 Since ApplicationResource currently wont cache the gear groups (TODO comment exists) we wont get hurt, any call to it would reload the gear groups. I usually "fix" this by force reloading the outdated resource as you mentioned above. I filed https://issues.jboss.org/browse/OSJC-130 for the missing caching even though it's very unlikely that we'll ever resolve it (moving to v3 client lib ahead)
|
I rebased, fixed the tests (corrected StandaloneCartridge#equals) and added IDeployedStandaloneCartridge#getAdditionalGearStorage() pushed the result to PR #167 |
2 participants
...e