adoroszlai commented on code in PR #7402: URL: https://github.com/apache/ozone/pull/7402#discussion_r1846979936
########## hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/VoidCallable.java: ########## @@ -0,0 +1,26 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.hadoop.hdds.utils; + +/** + * Defines a functional interface to call void returning function. + */ +@FunctionalInterface +public interface VoidCallable<EXCEPTION_TYPE extends Exception> { + void call() throws EXCEPTION_TYPE; Review Comment: Please use `CheckedRunnable`. ########## hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java: ########## @@ -175,7 +175,8 @@ private boolean canIgnoreException(Result result) { case CONTAINER_UNHEALTHY: case CLOSED_CONTAINER_IO: case DELETE_ON_OPEN_CONTAINER: - case UNSUPPORTED_REQUEST: // Blame client for sending unsupported request. + case UNSUPPORTED_REQUEST: + case CONTAINER_MISSING:// Blame client for sending unsupported request. Review Comment: Comment about `unsupported request` belongs to `UNSUPPORTED_REQUEST` case... ########## hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/ClosedContainerReplicator.java: ########## @@ -82,66 +84,74 @@ public class ClosedContainerReplicator extends BaseFreonGenerator implements private ContainerReplicator replicator; private Timer timer; + private ReferenceCountedHandle<MasterVolumeMetadataStore> masterVolumeMetadataStoreReferenceCountedDB; private List<ReplicationTask> replicationTasks; @Override public Void call() throws Exception { - OzoneConfiguration conf = createOzoneConfiguration(); + try { + OzoneConfiguration conf = createOzoneConfiguration(); - final Collection<String> datanodeStorageDirs = - HddsServerUtil.getDatanodeStorageDirs(conf); + final Collection<String> datanodeStorageDirs = + HddsServerUtil.getDatanodeStorageDirs(conf); Review Comment: This change is just wrapping existing code in `try-finally`, with new code added only in `finally`. You can reduce the change by: - rename existing `call()` to something else (using `oldCall()` here as example) - add new `call()` as: ```java public Void call() throws Exception { try { oldCall(); } finally { ... } } ``` This way indentation is unchanged, diff is reduced. ########## hadoop-ozone/dist/src/main/compose/compatibility/docker-config: ########## @@ -21,7 +21,7 @@ OZONE-SITE.XML_ozone.scm.datanode.ratis.volume.free-space.min=10MB OZONE-SITE.XML_ozone.scm.pipeline.creation.interval=30s OZONE-SITE.XML_ozone.scm.pipeline.owner.container.count=1 OZONE-SITE.XML_ozone.scm.names=scm -OZONE-SITE.XML_ozone.scm.datanode.id.dir=/data +OZONE-SITE.XML_ozone.scm.datanode.id.dir=/data/metadata Review Comment: Can you please explain this part of the change? ########## hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/AbstractStore.java: ########## @@ -0,0 +1,73 @@ +/* + * 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.hadoop.ozone.container.metadata; + +import com.google.common.annotations.VisibleForTesting; +import org.apache.hadoop.hdds.conf.ConfigurationSource; +import org.apache.hadoop.hdds.utils.db.BatchOperationHandler; +import org.apache.hadoop.hdds.utils.db.DBStore; + +import java.io.Closeable; +import java.io.IOException; + +/** + * Abstract Interface for interacting with datanode databases. + */ +public interface AbstractStore extends Closeable { Review Comment: Interfaces are already abstract, so both doc and interface name are very vague. Please try to find a better name. As far as I see, it's mostly for managing lifecycle of a `DBStore`. Also, this parent interface does not seem to be specific to datanodes. Should it be moved to `hadoop-hdds/framework`? ########## hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/MasterVolumeDBDefinition.java: ########## @@ -0,0 +1,72 @@ +package org.apache.hadoop.ozone.container.metadata; + +/* + * 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. + * + */ + +import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State; +import org.apache.hadoop.hdds.scm.ScmConfigKeys; +import org.apache.hadoop.hdds.utils.db.DBColumnFamilyDefinition; +import org.apache.hadoop.hdds.utils.db.DBDefinition; +import org.apache.hadoop.hdds.utils.db.LongCodec; +import org.apache.hadoop.hdds.utils.db.Proto2EnumCodec; +import org.apache.hadoop.ozone.OzoneConsts; + +import java.util.Map; + +/** + * Class for defining the schema for master volume in a datanode. Review Comment: What is a master volume? ########## hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBTestUtils.java: ########## @@ -0,0 +1,142 @@ +package org.apache.hadoop.hdds.utils.db; + +/* + * 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. + * + */ + +import org.apache.hadoop.hdds.utils.MetadataKeyFilters; + +import java.io.File; +import java.io.IOException; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; + +/** + * Util class for mocking DB interactions happening in various tests. + */ +public final class DBTestUtils { + + private DBTestUtils() { + + } + + public static <KEY, VALUE> Table<KEY, VALUE> getInMemoryTableForTest() { + return new Table<KEY, VALUE>() { + private final Map<KEY, VALUE> map = new ConcurrentHashMap<>(); Review Comment: Please create a non-anonymous class for the `Table` implementation, and drop `DBTestUtils`, which has no other code. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
