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/24 03:32:19 UTC
[GitHub] [geode] jinmeiliao commented on a change in pull request #5288: GEODE-8283: Provide REST interface for disk-store creation
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