You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/06/23 21:49:20 UTC
[GitHub] [geode] jmelchio opened a new pull request #5288: GEODE-8283: Provide REST interface for disk-store creation
jmelchio opened a new pull request #5288:
URL: https://github.com/apache/geode/pull/5288
- Provides create, get, list and delete operations
- Can create with locator only running
Co-Authored-By: Jason Huynh <jh...@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 build` run 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](http://www.apache.org/legal/resolved.html#category-a)?
### 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jmelchio commented on a change in pull request #5288: GEODE-8283: Provide REST interface for disk-store creation
Posted by GitBox <gi...@apache.org>.
jmelchio commented on a change in pull request #5288:
URL: https://github.com/apache/geode/pull/5288#discussion_r444931732
##########
File path: geode-management/src/main/java/org/apache/geode/management/configuration/DiskStore.java
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.geode.management.configuration;
+
+import java.util.List;
+import java.util.Objects;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.management.runtime.DiskStoreInfo;
+
+@Experimental
+public class DiskStore extends GroupableConfiguration<DiskStoreInfo> {
+ public static final String DISK_STORE_CONFIG_ENDPOINT = "/diskstores";
+
+ @JsonIgnore
+ private String id;
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jmelchio commented on a change in pull request #5288: GEODE-8283: Provide REST interface for disk-store creation
Posted by GitBox <gi...@apache.org>.
jmelchio commented on a change in pull request #5288:
URL: https://github.com/apache/geode/pull/5288#discussion_r445552302
##########
File path: geode-management/src/main/java/org/apache/geode/management/configuration/DiskStore.java
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.geode.management.configuration;
+
+import java.util.List;
+import java.util.Objects;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.management.runtime.DiskStoreInfo;
+
+@Experimental
+public class DiskStore extends GroupableConfiguration<DiskStoreInfo> {
+ public static final String DISK_STORE_CONFIG_ENDPOINT = "/diskstores";
+
+ @JsonIgnore
+ private String id;
Review comment:
We need it for setting up disk-stores in k8s.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5288: GEODE-8283: Provide REST interface for disk-store creation
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #5288:
URL: https://github.com/apache/geode/pull/5288#discussion_r444989870
##########
File path: geode-management/src/main/java/org/apache/geode/management/configuration/DiskStore.java
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.geode.management.configuration;
+
+import java.util.List;
+import java.util.Objects;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.management.runtime.DiskStoreInfo;
+
+@Experimental
+public class DiskStore extends GroupableConfiguration<DiskStoreInfo> {
Review comment:
If it returns null in the `get` method?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jmelchio commented on a change in pull request #5288: GEODE-8283: Provide REST interface for disk-store creation
Posted by GitBox <gi...@apache.org>.
jmelchio commented on a change in pull request #5288:
URL: https://github.com/apache/geode/pull/5288#discussion_r444930478
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/DiskStoreManager.java
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.geode.management.internal.configuration.mutators;
+
+import java.util.List;
+import java.util.stream.Collectors;
+
+import org.apache.commons.lang3.StringUtils;
+
+import org.apache.geode.cache.configuration.CacheConfig;
+import org.apache.geode.cache.configuration.DiskStoreType;
+import org.apache.geode.distributed.ConfigurationPersistenceService;
+import org.apache.geode.management.configuration.DiskStore;
+import org.apache.geode.management.internal.configuration.converters.DiskStoreConverter;
+
+public class DiskStoreManager extends CacheConfigurationManager<DiskStore> {
+ private final DiskStoreConverter diskStoreConverter = new DiskStoreConverter();
+
+ public DiskStoreManager(ConfigurationPersistenceService service) {
+ super(service);
+ }
+
+ @Override
+ public void add(DiskStore config, CacheConfig existing) {
+ List<DiskStoreType> diskStoreTypes = existing.getDiskStores();
+ if (diskStoreTypes.stream().noneMatch(diskStoreType -> diskStoreType.getName()
Review comment:
Okay, we'll check if we can drop these checks without side effects.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jmelchio commented on a change in pull request #5288: GEODE-8283: Provide REST interface for disk-store creation
Posted by GitBox <gi...@apache.org>.
jmelchio commented on a change in pull request #5288:
URL: https://github.com/apache/geode/pull/5288#discussion_r444973701
##########
File path: geode-management/src/main/java/org/apache/geode/management/configuration/DiskStore.java
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.geode.management.configuration;
+
+import java.util.List;
+import java.util.Objects;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.management.runtime.DiskStoreInfo;
+
+@Experimental
+public class DiskStore extends GroupableConfiguration<DiskStoreInfo> {
Review comment:
It appears the `DiskStoreRealizer` needs a concrete implementation of `RuntimeInfo`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jmelchio merged pull request #5288: GEODE-8283: Provide REST interface for disk-store creation
Posted by GitBox <gi...@apache.org>.
jmelchio merged pull request #5288:
URL: https://github.com/apache/geode/pull/5288
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5288: GEODE-8283: Provide REST interface for disk-store creation
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #5288:
URL: https://github.com/apache/geode/pull/5288#discussion_r444622138
##########
File path: geode-management/src/main/java/org/apache/geode/management/configuration/DiskStore.java
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.geode.management.configuration;
+
+import java.util.List;
+import java.util.Objects;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.management.runtime.DiskStoreInfo;
+
+@Experimental
+public class DiskStore extends GroupableConfiguration<DiskStoreInfo> {
+ public static final String DISK_STORE_CONFIG_ENDPOINT = "/diskstores";
+
+ @JsonIgnore
+ private String id;
Review 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.
##########
File path: geode-management/src/main/java/org/apache/geode/management/configuration/DiskDir.java
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.geode.management.configuration;
+
+import java.io.Serializable;
+
+import org.apache.geode.management.api.JsonSerializable;
+
+public class DiskDir implements Serializable, JsonSerializable {
+ private String name;
+ private String dirSize;
Review comment:
would an int/long be better?
##########
File path: geode-management/src/main/java/org/apache/geode/management/configuration/DiskStore.java
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.geode.management.configuration;
+
+import java.util.List;
+import java.util.Objects;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.management.runtime.DiskStoreInfo;
+
+@Experimental
+public class DiskStore extends GroupableConfiguration<DiskStoreInfo> {
Review 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.
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/DiskStoreManager.java
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.geode.management.internal.configuration.mutators;
+
+import java.util.List;
+import java.util.stream.Collectors;
+
+import org.apache.commons.lang3.StringUtils;
+
+import org.apache.geode.cache.configuration.CacheConfig;
+import org.apache.geode.cache.configuration.DiskStoreType;
+import org.apache.geode.distributed.ConfigurationPersistenceService;
+import org.apache.geode.management.configuration.DiskStore;
+import org.apache.geode.management.internal.configuration.converters.DiskStoreConverter;
+
+public class DiskStoreManager extends CacheConfigurationManager<DiskStore> {
+ private final DiskStoreConverter diskStoreConverter = new DiskStoreConverter();
+
+ public DiskStoreManager(ConfigurationPersistenceService service) {
+ super(service);
+ }
+
+ @Override
+ public void add(DiskStore config, CacheConfig existing) {
+ List<DiskStoreType> diskStoreTypes = existing.getDiskStores();
+ if (diskStoreTypes.stream().noneMatch(diskStoreType -> diskStoreType.getName()
+ .equals(config.getName()))) {
+ diskStoreTypes.add(diskStoreConverter.fromConfigObject(config));
+ }
+ }
+
+ @Override
+ public void update(DiskStore config, CacheConfig existing) {
+ throw new IllegalStateException("Not implemented");
+ }
+
+ @Override
+ public void delete(DiskStore config, CacheConfig existing) {
+ existing.getDiskStores().stream()
Review comment:
same as add, no need to check
##########
File path: geode-core/src/main/java/org/apache/geode/management/internal/configuration/mutators/DiskStoreManager.java
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.geode.management.internal.configuration.mutators;
+
+import java.util.List;
+import java.util.stream.Collectors;
+
+import org.apache.commons.lang3.StringUtils;
+
+import org.apache.geode.cache.configuration.CacheConfig;
+import org.apache.geode.cache.configuration.DiskStoreType;
+import org.apache.geode.distributed.ConfigurationPersistenceService;
+import org.apache.geode.management.configuration.DiskStore;
+import org.apache.geode.management.internal.configuration.converters.DiskStoreConverter;
+
+public class DiskStoreManager extends CacheConfigurationManager<DiskStore> {
+ private final DiskStoreConverter diskStoreConverter = new DiskStoreConverter();
+
+ public DiskStoreManager(ConfigurationPersistenceService service) {
+ super(service);
+ }
+
+ @Override
+ public void add(DiskStore config, CacheConfig existing) {
+ List<DiskStoreType> diskStoreTypes = existing.getDiskStores();
+ if (diskStoreTypes.stream().noneMatch(diskStoreType -> diskStoreType.getName()
Review 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`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5288: GEODE-8283: Provide REST interface for disk-store creation
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #5288:
URL: https://github.com/apache/geode/pull/5288#discussion_r445592976
##########
File path: geode-management/src/main/java/org/apache/geode/management/configuration/DiskStore.java
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.geode.management.configuration;
+
+import java.util.List;
+import java.util.Objects;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.management.runtime.DiskStoreInfo;
+
+@Experimental
+public class DiskStore extends GroupableConfiguration<DiskStoreInfo> {
+ public static final String DISK_STORE_CONFIG_ENDPOINT = "/diskstores";
+
+ @JsonIgnore
+ private String id;
Review 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5288: GEODE-8283: Provide REST interface for disk-store creation
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #5288:
URL: https://github.com/apache/geode/pull/5288#discussion_r445263810
##########
File path: geode-management/src/main/java/org/apache/geode/management/configuration/DiskStore.java
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.geode.management.configuration;
+
+import java.util.List;
+import java.util.Objects;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.management.runtime.DiskStoreInfo;
+
+@Experimental
+public class DiskStore extends GroupableConfiguration<DiskStoreInfo> {
+ public static final String DISK_STORE_CONFIG_ENDPOINT = "/diskstores";
+
+ @JsonIgnore
+ private String id;
Review 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5288: GEODE-8283: Provide REST interface for disk-store creation
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #5288:
URL: https://github.com/apache/geode/pull/5288#discussion_r445063302
##########
File path: geode-management/src/main/java/org/apache/geode/management/configuration/DiskStore.java
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.geode.management.configuration;
+
+import java.util.List;
+import java.util.Objects;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.management.runtime.DiskStoreInfo;
+
+@Experimental
+public class DiskStore extends GroupableConfiguration<DiskStoreInfo> {
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jmelchio commented on a change in pull request #5288: GEODE-8283: Provide REST interface for disk-store creation
Posted by GitBox <gi...@apache.org>.
jmelchio commented on a change in pull request #5288:
URL: https://github.com/apache/geode/pull/5288#discussion_r444930664
##########
File path: geode-management/src/main/java/org/apache/geode/management/configuration/DiskDir.java
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.geode.management.configuration;
+
+import java.io.Serializable;
+
+import org.apache.geode.management.api.JsonSerializable;
+
+public class DiskDir implements Serializable, JsonSerializable {
+ private String name;
+ private String dirSize;
Review comment:
Yes, probably better.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jmelchio commented on a change in pull request #5288: GEODE-8283: Provide REST interface for disk-store creation
Posted by GitBox <gi...@apache.org>.
jmelchio commented on a change in pull request #5288:
URL: https://github.com/apache/geode/pull/5288#discussion_r444928974
##########
File path: geode-management/src/main/java/org/apache/geode/management/configuration/DiskStore.java
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.geode.management.configuration;
+
+import java.util.List;
+import java.util.Objects;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.management.runtime.DiskStoreInfo;
+
+@Experimental
+public class DiskStore extends GroupableConfiguration<DiskStoreInfo> {
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jmelchio commented on a change in pull request #5288: GEODE-8283: Provide REST interface for disk-store creation
Posted by GitBox <gi...@apache.org>.
jmelchio commented on a change in pull request #5288:
URL: https://github.com/apache/geode/pull/5288#discussion_r445045526
##########
File path: geode-management/src/main/java/org/apache/geode/management/configuration/DiskStore.java
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.geode.management.configuration;
+
+import java.util.List;
+import java.util.Objects;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+
+import org.apache.geode.annotations.Experimental;
+import org.apache.geode.management.runtime.DiskStoreInfo;
+
+@Experimental
+public class DiskStore extends GroupableConfiguration<DiskStoreInfo> {
Review comment:
That would work but then we wouldn't even get a list of machines where the disk-stores live at run time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org