-
Notifications
You must be signed in to change notification settings - Fork 594
HDDS-13176. containerIds table value format change to proto from string #8589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c1d94aa
0eabb6e
d39a809
44e2cb8
15f8143
9e5522a
99b6ef3
82eeb0d
1f39476
bd5f494
f89fc31
ec8e08f
fc5ba25
b3c3c12
6a1d9b2
2f8040c
788aad7
1052e97
6e22964
2850588
ec08da8
fa05a56
d14ae78
8b28beb
073b74b
b4dc071
c1ff360
9af70bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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.hadoop.ozone.container.metadata; | ||
|
|
||
| import java.util.function.Supplier; | ||
| import net.jcip.annotations.Immutable; | ||
| import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos; | ||
| import org.apache.hadoop.hdds.utils.db.Codec; | ||
| import org.apache.hadoop.hdds.utils.db.DelegatedCodec; | ||
| import org.apache.hadoop.hdds.utils.db.Proto3Codec; | ||
| import org.apache.ratis.util.MemoizedSupplier; | ||
|
|
||
| /** | ||
| * ContainerCreateInfo is a class that holds information about the state and other information on creation | ||
| * This class is immutable. | ||
| */ | ||
| @Immutable | ||
| public final class ContainerCreateInfo { | ||
| private static final Codec<ContainerCreateInfo> CODEC = new DelegatedCodec<>( | ||
| Proto3Codec.get(ContainerProtos.ContainerCreateInfo.getDefaultInstance()), | ||
| ContainerCreateInfo::getFromProtobuf, ContainerCreateInfo::getProtobuf, | ||
| ContainerCreateInfo.class); | ||
|
|
||
| private final ContainerProtos.ContainerDataProto.State state; | ||
| private final Supplier<ContainerProtos.ContainerCreateInfo> proto; | ||
|
|
||
| public static Codec<ContainerCreateInfo> getCodec() { | ||
| return CODEC; | ||
| } | ||
|
|
||
| public static Codec<ContainerCreateInfo> getNewCodec() { | ||
| return CODEC; | ||
| } | ||
|
|
||
| private ContainerCreateInfo(ContainerProtos.ContainerDataProto.State state) { | ||
| this.state = state; | ||
| this.proto = MemoizedSupplier.valueOf( | ||
| () -> ContainerProtos.ContainerCreateInfo.newBuilder().setState(state).build()); | ||
| } | ||
|
|
||
| /** | ||
| * Factory method for creation of ContainerCreateInfo. | ||
| * @param state State | ||
| * @return ContainerCreateInfo. | ||
| */ | ||
| public static ContainerCreateInfo valueOf(final ContainerProtos.ContainerDataProto.State state) { | ||
| return new ContainerCreateInfo(state); | ||
| } | ||
|
|
||
| public ContainerProtos.ContainerCreateInfo getProtobuf() { | ||
| return proto.get(); | ||
| } | ||
|
|
||
| public static ContainerCreateInfo getFromProtobuf(ContainerProtos.ContainerCreateInfo proto) { | ||
| return ContainerCreateInfo.valueOf(proto.getState()); | ||
| } | ||
|
|
||
| public ContainerProtos.ContainerDataProto.State getState() { | ||
| return state; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -22,21 +22,28 @@ | |||||||||||||||||||||||||||
| import java.util.concurrent.ConcurrentHashMap; | ||||||||||||||||||||||||||||
| import java.util.concurrent.ConcurrentMap; | ||||||||||||||||||||||||||||
| import org.apache.hadoop.hdds.conf.ConfigurationSource; | ||||||||||||||||||||||||||||
| import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos; | ||||||||||||||||||||||||||||
| import org.apache.hadoop.hdds.scm.container.ContainerID; | ||||||||||||||||||||||||||||
| import org.apache.hadoop.hdds.upgrade.HDDSLayoutFeature; | ||||||||||||||||||||||||||||
| import org.apache.hadoop.hdds.utils.db.CodecException; | ||||||||||||||||||||||||||||
| import org.apache.hadoop.hdds.utils.db.DBStore; | ||||||||||||||||||||||||||||
| import org.apache.hadoop.hdds.utils.db.DBStoreBuilder; | ||||||||||||||||||||||||||||
| import org.apache.hadoop.hdds.utils.db.DelegatedCodec; | ||||||||||||||||||||||||||||
| import org.apache.hadoop.hdds.utils.db.RocksDatabaseException; | ||||||||||||||||||||||||||||
| import org.apache.hadoop.hdds.utils.db.StringCodec; | ||||||||||||||||||||||||||||
| import org.apache.hadoop.hdds.utils.db.Table; | ||||||||||||||||||||||||||||
| import org.apache.hadoop.hdds.utils.db.managed.ManagedDBOptions; | ||||||||||||||||||||||||||||
| import org.apache.hadoop.ozone.container.upgrade.VersionedDatanodeFeatures; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * Class for interacting with database in the master volume of a datanode. | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| public final class WitnessedContainerMetadataStoreImpl extends AbstractRDBStore<WitnessedContainerDBDefinition> | ||||||||||||||||||||||||||||
| implements WitnessedContainerMetadataStore { | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| private Table<ContainerID, String> containerIdsTable; | ||||||||||||||||||||||||||||
| private Table<ContainerID, ContainerCreateInfo> containerCreateInfoTable; | ||||||||||||||||||||||||||||
| private PreviousVersionTables previousVersionTables; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| private static final ConcurrentMap<String, WitnessedContainerMetadataStore> INSTANCES = | ||||||||||||||||||||||||||||
| new ConcurrentHashMap<>(); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
@@ -67,13 +74,53 @@ private WitnessedContainerMetadataStoreImpl(ConfigurationSource config, boolean | |||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||
| protected DBStore initDBStore(DBStoreBuilder dbStoreBuilder, ManagedDBOptions options, ConfigurationSource config) | ||||||||||||||||||||||||||||
| throws RocksDatabaseException, CodecException { | ||||||||||||||||||||||||||||
| previousVersionTables = new PreviousVersionTables(); | ||||||||||||||||||||||||||||
| previousVersionTables.addTables(dbStoreBuilder); | ||||||||||||||||||||||||||||
| final DBStore dbStore = dbStoreBuilder.build(); | ||||||||||||||||||||||||||||
| this.containerIdsTable = this.getDbDef().getContainerIdsTable().getTable(dbStore); | ||||||||||||||||||||||||||||
| previousVersionTables.init(dbStore); | ||||||||||||||||||||||||||||
| this.containerCreateInfoTable = this.getDbDef().getContainerCreateInfoTableDef().getTable(dbStore); | ||||||||||||||||||||||||||||
| return dbStore; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||
| public Table<ContainerID, String> getContainerIdsTable() { | ||||||||||||||||||||||||||||
| return containerIdsTable; | ||||||||||||||||||||||||||||
| public Table<ContainerID, ContainerCreateInfo> getContainerCreateInfoTable() { | ||||||||||||||||||||||||||||
| if (!VersionedDatanodeFeatures.isFinalized(HDDSLayoutFeature.WITNESSED_CONTAINER_DB_PROTO_VALUE)) { | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let us create a wrapper Table implementation which would automatically get updated to the finalized table once the layout upgrade is finalized. We can return the wrapped Table interface which would mean the caller of this method can keep this value in memory and we wouldn't have to update anything on the caller behaviour. Right now the caller is having to call witnessedMetadataStore.getContainerCreateInfoTable().put or delete everytime we can completely get this abstracted out.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could have this Wrapped object present as member in the class itself. So on finalize all we need to update is a boolean value for the present in the wrapped table.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can lead to bugs if called incorrectly. It is important that we abstract this out.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Created above JIRA for reference, I do not agree for caching table, as it need re-update after upgrade or other cases in future to update the reference. It can be more discussed if have real value.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have multiple instances in the code base where we cache the reference to the table. The expectation from what I understand is that once the metadata store is initialized the caller is free to cache the reference right now this model doesn't prevent that. We should either come up with a model which prevents caching the reference altogether which would mean redesigning the interface or we need to make the implementation abstracted so that it is fool proof to reference caching.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A few instances... ozone/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ListIterator.java Lines 300 to 302 in 5d1b43d
Lines 336 to 339 in 48c985f
Lines 60 to 65 in 48c985f
Our entire code base is spread with such use cases. If you are proposing something different then we should design the interface to prevent future bugs.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can be taken up with the JIRA as mentioned above for designing and defining interface |
||||||||||||||||||||||||||||
| return previousVersionTables.getContainerIdsTable(); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| return containerCreateInfoTable; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| public PreviousVersionTables getPreviousVersionTables() { | ||||||||||||||||||||||||||||
| return previousVersionTables; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * this will hold old version tables required during upgrade, and these are initialized based on version only. | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
| public static class PreviousVersionTables { | ||||||||||||||||||||||||||||
| private static final String CONTAINER_IDS_STR_VAL_TABLE = "containerIds"; | ||||||||||||||||||||||||||||
| private Table<ContainerID, ContainerCreateInfo> containerIdsTable; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| public PreviousVersionTables() { | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| public void addTables(DBStoreBuilder dbStoreBuilder) { | ||||||||||||||||||||||||||||
| if (!VersionedDatanodeFeatures.isFinalized(HDDSLayoutFeature.WITNESSED_CONTAINER_DB_PROTO_VALUE)) { | ||||||||||||||||||||||||||||
| dbStoreBuilder.addTable(CONTAINER_IDS_STR_VAL_TABLE); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| public void init(DBStore dbStore) throws RocksDatabaseException, CodecException { | ||||||||||||||||||||||||||||
| if (!VersionedDatanodeFeatures.isFinalized(HDDSLayoutFeature.WITNESSED_CONTAINER_DB_PROTO_VALUE)) { | ||||||||||||||||||||||||||||
| this.containerIdsTable = dbStore.getTable(CONTAINER_IDS_STR_VAL_TABLE, ContainerID.getCodec(), | ||||||||||||||||||||||||||||
| new DelegatedCodec<>(StringCodec.get(), | ||||||||||||||||||||||||||||
| (strVal) -> ContainerCreateInfo.valueOf(ContainerProtos.ContainerDataProto.State.valueOf(strVal)), | ||||||||||||||||||||||||||||
| (obj) -> obj.getState().name(), ContainerCreateInfo.class)); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| public Table<ContainerID, ContainerCreateInfo> getContainerIdsTable() { | ||||||||||||||||||||||||||||
| return containerIdsTable; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.