smengcl commented on code in PR #3082:
URL: https://github.com/apache/ozone/pull/3082#discussion_r953676945
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/ReconContainerMetadataManagerImpl.java:
##########
@@ -515,4 +547,84 @@ public void commitBatchOperation(RDBBatchOperation
rdbBatchOperation)
throws IOException {
this.containerDbStore.commitBatchOperation(rdbBatchOperation);
}
+
+ /**
+ * Use the DB's prefix seek iterator to start the scan from the given
+ * key prefix and key version.
+ *
+ * @param keyPrefix the given keyPrefix.
+ * @param keyVersion the given keyVersion.
+ * @return Map of (KeyPrefixContainer, Integer).
+ */
+ @Override
+ public Map<KeyPrefixContainer, Integer> getContainerForKeyPrefixes(
+ String keyPrefix, long keyVersion) throws IOException {
+
+ Map<KeyPrefixContainer, Integer> containers = new LinkedHashMap<>();
+ TableIterator<KeyPrefixContainer, ? extends KeyValue<KeyPrefixContainer,
+ Integer>> keyIterator = keyContainerTable.iterator();
Review Comment:
Need to close DB table iterator after use.
Either do `keyIterator.close()` before each `return`, or use
try-with-resource (recommended).
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/KeyPrefixContainerCodec.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.ozone.recon.spi.impl;
+
+import com.google.common.base.Preconditions;
+import com.google.common.primitives.Longs;
+import org.apache.commons.lang3.ArrayUtils;
+import org.apache.hadoop.hdds.utils.db.Codec;
+import org.apache.hadoop.ozone.recon.api.types.KeyPrefixContainer;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+
+import static org.apache.commons.compress.utils.CharsetNames.UTF_8;
+
+/**
+ * Codec to encode KeyPrefixContainer as byte array.
+ */
+public class KeyPrefixContainerCodec implements Codec<KeyPrefixContainer> {
+
+ private static final String KEY_DELIMITER = "_";
+
+ @Override
+ public byte[] toPersistedFormat(KeyPrefixContainer keyPrefixContainer)
+ throws IOException {
+ Preconditions.checkNotNull(keyPrefixContainer,
+ "Null object can't be converted to byte array.");
+ byte[] keyPrefixBytes = keyPrefixContainer.getKeyPrefix().getBytes(UTF_8);
+
+ //Prefix seek can be done only with keyPrefix. In that case, we can
+ // expect the version and the containerId to be undefined.
+ if (keyPrefixContainer.getKeyVersion() != -1) {
+ keyPrefixBytes = ArrayUtils.addAll(keyPrefixBytes, KEY_DELIMITER
+ .getBytes(UTF_8));
+ keyPrefixBytes = ArrayUtils.addAll(keyPrefixBytes, Longs.toByteArray(
+ keyPrefixContainer.getKeyVersion()));
+ }
+
+ if (keyPrefixContainer.getContainerId() != -1) {
+ keyPrefixBytes = ArrayUtils.addAll(keyPrefixBytes, KEY_DELIMITER
+ .getBytes(UTF_8));
+ keyPrefixBytes = ArrayUtils.addAll(keyPrefixBytes, Longs.toByteArray(
+ keyPrefixContainer.getContainerId()));
+ }
+
+ return keyPrefixBytes;
+ }
+
+ @Override
+ public KeyPrefixContainer fromPersistedFormat(byte[] rawData)
+ throws IOException {
+
+ // When reading from byte[], we can always expect to have the key, version
+ // and version parts in the byte array.
+ byte[] keyBytes = ArrayUtils.subarray(rawData,
+ 0, rawData.length - Long.BYTES * 2 - 2);
Review Comment:
We are encoding and decoding the DB entry value manually, with error-prone
offset calculations here. But since `ContainerKeyPrefixCodec` was already doing
this, this should be acceptable.
I wonder if it makes sense to just add a new proto message type, so we can
leverage protobuf instead? If it is worth it I think we should, depending on
how much more efficient (spatial and time) it can be if we switch to protobuf.
Just trying to open up a discussion here. IMO it is fine use manual
encode/decode here to make it consistent with the `containerKeyTable` we
already have. Maybe in a future jira we can switch both tables to use protobuf
to serialize the persisted DB value and call those tables `_V2`.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/ReconContainerMetadataManagerImpl.java:
##########
@@ -97,13 +102,19 @@ public void
reinitWithNewContainerDataFromOm(Map<ContainerKeyPrefix, Integer>
throws IOException {
// clear and re-init all container-related tables
truncateTable(this.containerKeyTable);
+ truncateTable(this.keyContainerTable);
truncateTable(this.containerKeyCountTable);
initializeTables();
if (containerKeyPrefixCounts != null) {
+ KeyPrefixContainer tmpKeyPreifxContainer;
Review Comment:
nit: typo
```suggestion
KeyPrefixContainer tmpKeyPrefixContainer;
```
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/spi/ReconContainerMetadataManager.java:
##########
@@ -235,5 +236,21 @@ void batchDeleteContainerMapping(BatchOperation batch,
*/
void commitBatchOperation(RDBBatchOperation rdbBatchOperation)
throws IOException;
+
+ /**
+ * Get iterator to the entire Key_Container DB.
Review Comment:
nit: alignment
```suggestion
* Get iterator to the entire Key_Container DB.
```
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/KeyPrefixContainer.java:
##########
@@ -0,0 +1,100 @@
+/*
+ * 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.ozone.recon.api.types;
+
+/**
+ * Class to encapsulate the Key information needed for the Recon container DB.
+ * Currently, it is the whole key + key version and the containerId.
+ */
+public class KeyPrefixContainer {
+
+ private String keyPrefix;
+ private long keyVersion = -1;
+ private long containerId = -1;
+
+ public KeyPrefixContainer(String keyPrefix, long keyVersion) {
+ this.keyPrefix = keyPrefix;
+ this.keyVersion = keyVersion;
+ }
+
+ public KeyPrefixContainer(String keyPrefix, long keyVersion,
+ long containerId) {
+ this.keyPrefix = keyPrefix;
+ this.keyVersion = keyVersion;
+ this.containerId = containerId;
+ }
+
+ public KeyPrefixContainer(ContainerKeyPrefix containerKeyPrefix) {
+ this.keyPrefix = containerKeyPrefix.getKeyPrefix();
+ this.keyVersion = containerKeyPrefix.getKeyVersion();
+ this.containerId = containerKeyPrefix.getContainerId();
+ }
+
+ public KeyPrefixContainer(String keyPrefix) {
+ this.keyPrefix = keyPrefix;
+ }
+
+ public String getKeyPrefix() {
+ return keyPrefix;
+ }
+
+ public void setKeyPrefix(String keyPrefix) {
+ this.keyPrefix = keyPrefix;
+ }
+
+ public long getKeyVersion() {
+ return keyVersion;
+ }
+
+ public void setKeyVersion(long keyVersion) {
+ this.keyVersion = keyVersion;
+ }
+
+ public long getContainerId() {
+ return containerId;
+ }
+
+ public void setContainerId(long containerId) {
+ this.containerId = containerId;
+ }
+
+ public ContainerKeyPrefix toContainerKeyPrefix() {
+ return new ContainerKeyPrefix(this.containerId,
+ this.keyPrefix, this.keyVersion);
+ }
+
+ @Override
+ public boolean equals(Object o) {
+
+ if (!(o instanceof KeyPrefixContainer)) {
+ return false;
+ }
+ KeyPrefixContainer that = (KeyPrefixContainer) o;
+ return (this.containerId == that.containerId) &&
+ this.keyPrefix.equals(that.keyPrefix) &&
+ this.keyVersion == that.keyVersion;
+ }
+
+ @Override
+ public int hashCode() {
+ return Long.valueOf(containerId).hashCode() + 13 * keyPrefix.hashCode() +
+ 17 * Long.valueOf(keyVersion).hashCode();
Review Comment:
I wondered why existing `ContainerKeyPrefix` did this.
Does this have any advantage over:
```java
return Objects.hash(containerId, keyPrefix, keyVersion);
```
?
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/spi/impl/ReconContainerMetadataManagerImpl.java:
##########
@@ -515,4 +547,84 @@ public void commitBatchOperation(RDBBatchOperation
rdbBatchOperation)
throws IOException {
this.containerDbStore.commitBatchOperation(rdbBatchOperation);
}
+
+ /**
+ * Use the DB's prefix seek iterator to start the scan from the given
+ * key prefix and key version.
+ *
+ * @param keyPrefix the given keyPrefix.
+ * @param keyVersion the given keyVersion.
+ * @return Map of (KeyPrefixContainer, Integer).
+ */
+ @Override
+ public Map<KeyPrefixContainer, Integer> getContainerForKeyPrefixes(
+ String keyPrefix, long keyVersion) throws IOException {
+
+ Map<KeyPrefixContainer, Integer> containers = new LinkedHashMap<>();
+ TableIterator<KeyPrefixContainer, ? extends KeyValue<KeyPrefixContainer,
+ Integer>> keyIterator = keyContainerTable.iterator();
+ KeyPrefixContainer seekKey;
+ if (keyVersion != -1) {
+ seekKey = new KeyPrefixContainer(keyPrefix, keyVersion);
+ } else {
+ seekKey = new KeyPrefixContainer(keyPrefix);
+ }
+ KeyValue<KeyPrefixContainer, Integer> seekKeyValue =
+ keyIterator.seek(seekKey);
+
+ // check if RocksDB was able to seek correctly to the given key prefix
+ // if not, then return empty result
+ // In case of an empty prevKeyPrefix, all the keys in the container are
+ // returned
+ if (seekKeyValue == null ||
+ (keyVersion != -1 &&
+ seekKeyValue.getKey().getKeyVersion() != keyVersion)) {
+ return containers;
+ }
+
+ while (keyIterator.hasNext()) {
+ KeyValue<KeyPrefixContainer, Integer> keyValue = keyIterator.next();
+ KeyPrefixContainer keyPrefixContainer = keyValue.getKey();
+
+ // The prefix seek only guarantees that the iterator's head will be
+ // positioned at the first prefix match. We still have to check the key
+ // prefix.
+ if (keyPrefixContainer.getKeyPrefix().equals(keyPrefix)) {
+ if (keyPrefixContainer.getContainerId() != -1 &&
+ (keyVersion == -1 ||
+ keyPrefixContainer.getKeyVersion() == keyVersion)) {
+ containers.put(new KeyPrefixContainer(keyPrefix,
+ keyPrefixContainer.getKeyVersion(),
+ keyPrefixContainer.getContainerId()),
+ keyValue.getValue());
+ } else {
+ LOG.warn("Null container returned for keyPrefix = {}," +
+ " keyVersion = {} ", keyPrefix, keyVersion);
+ }
+ } else {
+ break; //Break when the first mismatch occurs.
Review Comment:
nit: comment style
```suggestion
// Break on first mismatch
break;
```
--
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]
