GEODE-8283: Provide REST interface for disk-store creation #5288
Conversation
| public static final String DISK_STORE_CONFIG_ENDPOINT = "/diskstores"; | ||
|
|
||
| @JsonIgnore | ||
| private String id; |
There was a problem hiding this comment.
Does it need both id and name? Are all the attributes necessary in the rest interface? Rest interface is supposed to be a more simplified/opinionated interface than the xml. It doesn't have to repeat everything that xml object has. Rather only focus on the attributes that users would need to configure most. If having it there would confuse users rather than help them, better not provide it in the rest interface.
There was a problem hiding this comment.
We weren't sure what would and would not be used often. If we have someone who can sound off on what to include I'm happy to trim the list of properties and revert to defaults where we don't have them.
There was a problem hiding this comment.
What's your use case for needing these rest api? For now I would suggest just expose those attributes that you needed. It's always easy to add attributes to the config objects
There was a problem hiding this comment.
We need it for setting up disk-stores in k8s.
There was a problem hiding this comment.
Do you need to configure all the attributes in k8s? I am asking because of what we did for Region config object, we only expose what's critical to configure the region to simplify user experience. I would hope Diskstore would do the same. Maybe talk to Gang to find out what are the most used attributes when configuring diskstore and what are the user's pain point given the current set of options in gfsh
|
|
||
| public class DiskDir implements Serializable, JsonSerializable { | ||
| private String name; | ||
| private String dirSize; |
There was a problem hiding this comment.
would an int/long be better?
There was a problem hiding this comment.
Yes, probably better.
| @Override | ||
| public void add(DiskStore config, CacheConfig existing) { | ||
| List<DiskStoreType> diskStoreTypes = existing.getDiskStores(); | ||
| if (diskStoreTypes.stream().noneMatch(diskStoreType -> diskStoreType.getName() |
There was a problem hiding this comment.
I don't think you need the check here. the pipeline should already make sure that there is no existing one that has the same id exists before it's calling this add
There was a problem hiding this comment.
Okay, we'll check if we can drop these checks without side effects.
|
|
||
| @Override | ||
| public void delete(DiskStore config, CacheConfig existing) { | ||
| existing.getDiskStores().stream() |
There was a problem hiding this comment.
same as add, no need to check
| import org.apache.geode.management.runtime.DiskStoreInfo; | ||
|
|
||
| @Experimental | ||
| public class DiskStore extends GroupableConfiguration<DiskStoreInfo> { |
There was a problem hiding this comment.
If DiskStoreInfo is an empty class (no runtime info to gather for this configuration object), then you can declare this DiskStore as public class DiskStore extends GroupableConfiguration<RuntimeInfo>. This way, for get/list, the cms will not even issue the functions to the servers to try to retrieve an empty object.
There was a problem hiding this comment.
In its current configuration it does list the servers to which the DiskStore has been applied. I'll try and see if this continues when we roll it into the DiskStore class. If that is the case we can drop the DiskStoreInfo class.
There was a problem hiding this comment.
It appears the DiskStoreRealizer needs a concrete implementation of RuntimeInfo.
There was a problem hiding this comment.
If it returns null in the get method?
There was a problem hiding this comment.
That would work but then we wouldn't even get a list of machines where the disk-stores live at run time.
There was a problem hiding this comment.
Ah, I see, you don't need further diskstore info on each server, but you do need the server id. I guess you can leave it as is.
jmelchio
force-pushed
the
disk-store-rest-api
branch
from
236fa9e
to
02d1049
Compare
- Provides create, get, list and delete operations - Can create with locator only running Co-Authored-By: Jason Huynh <jhuynh@vmware.com>
* Updating assembly validation txt Trying to remove disk store folders after test * Attempt to clear out disk dirs between runs * Added temporary folder and forced disk dirs to use the absolute path
* Updating assembly validation txt Trying to remove disk store folders after test * Attempt to clear out disk dirs between runs * Added temporary folder and forced disk dirs to use the absolute path * Added tests for groups Prevent removal of disk store from specific groups to be consistent with region api
- spottless apply
- Rebase against develop
jmelchio
force-pushed
the
disk-store-rest-api
branch
from
43a627a
to
97ba5d0
Compare
Co-Authored-By: Jason Huynh jhuynh@vmware.com
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop)?Is your initial contribution a single, squashed commit?
Does
gradlew buildrun cleanly?Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.