Skip to content

HDDS-12541. Change ContainerID to value-based. #8044

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.

Sign up for GitHub

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

Merged
adoroszlai merged 4 commits into from
Mar 11, 2025
Merged

HDDS-12541. Change ContainerID to value-based. #8044

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3d734b6
HDDS-12541. Change ContainerID to value-based.
szetszwo Mar 10, 2025
254c5af
Fix checkstyle
szetszwo Mar 10, 2025
e998018
Use MemoizedSupplier
szetszwo Mar 11, 2025
509f6a5
Address review comments.
szetszwo Mar 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 18 additions & 22 deletions hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerID.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@
package org.apache.hadoop.hdds.scm.container;

import com.google.common.base.Preconditions;
import org.apache.commons.lang3.builder.CompareToBuilder;
import org.apache.commons.lang3.builder.EqualsBuilder;
import org.apache.commons.lang3.builder.HashCodeBuilder;
import jakarta.annotation.Nonnull;
import java.util.Objects;
import java.util.function.Supplier;
import net.jcip.annotations.Immutable;
import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
import org.apache.hadoop.hdds.utils.db.Codec;
import org.apache.hadoop.hdds.utils.db.DelegatedCodec;
import org.apache.hadoop.hdds.utils.db.LongCodec;
import org.apache.ratis.util.MemoizedSupplier;

/**
* Container ID is an integer that is a value between 1..MAX_CONTAINER ID.
Expand All @@ -34,6 +36,7 @@
* <p>
* This class is immutable.
*/
@Immutable
public final class ContainerID implements Comparable<ContainerID> {
private static final Codec<ContainerID> CODEC = new DelegatedCodec<>(
LongCodec.get(), ContainerID::valueOf, c -> c.id,
Expand All @@ -46,16 +49,20 @@ public static Codec<ContainerID> getCodec() {
}

private final long id;
private final Supplier<HddsProtos.ContainerID> proto;
private final Supplier<Integer> hash;

/**
* Constructs ContainerID.
*
* @param id int
*/
public ContainerID(long id) {
private ContainerID(long id) {
Preconditions.checkState(id >= 0,
"Container ID should be positive. %s.", id);
this.id = id;
this.proto = MemoizedSupplier.valueOf(() -> HddsProtos.ContainerID.newBuilder().setId(id).build());
this.hash = MemoizedSupplier.valueOf(() -> 61 * 71 + Long.hashCode(id));
}

/**
Expand All @@ -80,16 +87,12 @@ public long getId() {
return id;
}

/**
* Use proto message.
*/
@Deprecated
public byte[] getBytes() {
public static byte[] getBytes(long id) {
return LongCodec.get().toPersistedFormat(id);
}

public HddsProtos.ContainerID getProtobuf() {
return HddsProtos.ContainerID.newBuilder().setId(id).build();
return proto.get();
}

public static ContainerID getFromProtobuf(HddsProtos.ContainerID proto) {
Expand All @@ -107,25 +110,18 @@ public boolean equals(final Object o) {
}

final ContainerID that = (ContainerID) o;

return new EqualsBuilder()
.append(id, that.id)
.isEquals();
return this.id == that.id;
}

@Override
public int hashCode() {
return new HashCodeBuilder(61, 71)
.append(id)
.toHashCode();
return hash.get();
}

@Override
public int compareTo(final ContainerID that) {
Preconditions.checkNotNull(that);
return new CompareToBuilder()
.append(this.id, that.id)
.build();
public int compareTo(@Nonnull final ContainerID that) {
Objects.requireNonNull(that, "that == null");
return Long.compare(this.id, that.id);
}

@Override
Expand Down
21 changes: 21 additions & 0 deletions hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@

import java.time.Duration;
import java.time.Instant;
import java.util.concurrent.ThreadLocalRandom;
import org.apache.commons.lang3.builder.HashCodeBuilder;
import org.apache.hadoop.hdds.client.ECReplicationConfig;
import org.apache.hadoop.hdds.client.RatisReplicationConfig;
import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
Expand All @@ -40,6 +42,25 @@
*/

public class TestContainerInfo {
static int oldHash(long id) {
return new HashCodeBuilder(61, 71)
.append(id)
.toHashCode();
}

static void assertHash(long value) {
final ContainerID id = ContainerID.valueOf(value);
assertEquals(oldHash(value), id.hashCode(), id::toString);
}

@Test
void testContainIdHash() {
for (int i = 0; i < 100; i++) {
assertHash(i);
final long id = ThreadLocalRandom.current().nextLong(Long.MAX_VALUE);
assertHash(id);
}
}

@Test
void getProtobufRatis() {
Expand Down
22 changes: 11 additions & 11 deletions ...mmon/src/test/java/org/apache/hadoop/hdds/scm/container/TestReplicationManagerReport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,13 @@ void testJsonOutput() throws IOException {

report.incrementAndSample(
ReplicationManagerReport.HealthState.UNDER_REPLICATED,
new ContainerID(1));
ContainerID.valueOf(1));
report.incrementAndSample(
ReplicationManagerReport.HealthState.UNDER_REPLICATED,
new ContainerID(2));
ContainerID.valueOf(2));
report.incrementAndSample(
ReplicationManagerReport.HealthState.OVER_REPLICATED,
new ContainerID(3));
ContainerID.valueOf(3));
report.setComplete();

String jsonString = JsonUtils.toJsonStringWithDefaultPrettyPrinter(report);
Expand Down Expand Up @@ -124,13 +124,13 @@ void testJsonOutput() throws IOException {
void testContainerIDsCanBeSampled() {
report.incrementAndSample(
ReplicationManagerReport.HealthState.UNDER_REPLICATED,
new ContainerID(1));
ContainerID.valueOf(1));
report.incrementAndSample(
ReplicationManagerReport.HealthState.UNDER_REPLICATED,
new ContainerID(2));
ContainerID.valueOf(2));
report.incrementAndSample(
ReplicationManagerReport.HealthState.OVER_REPLICATED,
new ContainerID(3));
ContainerID.valueOf(3));

assertEquals(2,
report.getStat(ReplicationManagerReport.HealthState.UNDER_REPLICATED));
Expand All @@ -141,13 +141,13 @@ void testContainerIDsCanBeSampled() {

List<ContainerID> sample =
report.getSample(ReplicationManagerReport.HealthState.UNDER_REPLICATED);
assertEquals(new ContainerID(1), sample.get(0));
assertEquals(new ContainerID(2), sample.get(1));
assertEquals(ContainerID.valueOf(1), sample.get(0));
assertEquals(ContainerID.valueOf(2), sample.get(1));
assertEquals(2, sample.size());

sample =
report.getSample(ReplicationManagerReport.HealthState.OVER_REPLICATED);
assertEquals(new ContainerID(3), sample.get(0));
assertEquals(ContainerID.valueOf(3), sample.get(0));
assertEquals(1, sample.size());

sample =
Expand All @@ -160,13 +160,13 @@ void testSamplesAreLimited() {
for (int i = 0; i < ReplicationManagerReport.SAMPLE_LIMIT * 2; i++) {
report.incrementAndSample(
ReplicationManagerReport.HealthState.UNDER_REPLICATED,
new ContainerID(i));
ContainerID.valueOf(i));
}
List<ContainerID> sample =
report.getSample(ReplicationManagerReport.HealthState.UNDER_REPLICATED);
assertEquals(ReplicationManagerReport.SAMPLE_LIMIT, sample.size());
for (int i = 0; i < ReplicationManagerReport.SAMPLE_LIMIT; i++) {
assertEquals(new ContainerID(i), sample.get(i));
assertEquals(ContainerID.valueOf(i), sample.get(i));
}
}

Expand Down
2 changes: 1 addition & 1 deletion ...java/org/apache/hadoop/ozone/container/ec/reconstruction/ECReconstructionCoordinator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ private SortedMap<Long, BlockData[]> getBlockDataMap(long containerID,

SortedMap<Long, BlockData[]> resultMap = new TreeMap<>();
Token<ContainerTokenIdentifier> containerToken =
tokenHelper.getContainerToken(new ContainerID(containerID));
tokenHelper.getContainerToken(ContainerID.valueOf(containerID));

Iterator<Map.Entry<Integer, DatanodeDetails>> iterator =
sourceNodeMap.entrySet().iterator();
Expand Down
44 changes: 22 additions & 22 deletions ...dds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/TestSCMCommonPlacementPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ public void testReplicasToFixMisreplicationWithOneMisreplication() {
List<DatanodeDetails> replicaDns = Stream.of(0, 1, 2, 3, 5)
.map(list::get).collect(Collectors.toList());
List<ContainerReplica> replicas =
HddsTestUtils.getReplicasWithReplicaIndex(new ContainerID(1),
HddsTestUtils.getReplicasWithReplicaIndex(ContainerID.valueOf(1),
CLOSED, 0, 0, 0, replicaDns);
testReplicasToFixMisreplication(replicas, dummyPlacementPolicy, 1,
ImmutableMap.of(racks.get(0), 1));
Expand All @@ -153,7 +153,7 @@ public void testReplicasToFixMisreplicationWithTwoMisreplication() {
List<DatanodeDetails> replicaDns = Stream.of(0, 1, 2, 3, 5)
.map(list::get).collect(Collectors.toList());
List<ContainerReplica> replicas =
HddsTestUtils.getReplicasWithReplicaIndex(new ContainerID(1),
HddsTestUtils.getReplicasWithReplicaIndex(ContainerID.valueOf(1),
CLOSED, 0, 0, 0, replicaDns);
testReplicasToFixMisreplication(replicas, dummyPlacementPolicy, 2,
ImmutableMap.of(racks.get(0), 2));
Expand All @@ -174,7 +174,7 @@ public void testReplicasToFixMisreplicationWithThreeMisreplication() {
List<DatanodeDetails> replicaDns = Stream.of(0, 1, 2, 3, 5)
.map(list::get).collect(Collectors.toList());
List<ContainerReplica> replicas =
HddsTestUtils.getReplicasWithReplicaIndex(new ContainerID(1),
HddsTestUtils.getReplicasWithReplicaIndex(ContainerID.valueOf(1),
CLOSED, 0, 0, 0, replicaDns);
testReplicasToFixMisreplication(replicas, dummyPlacementPolicy, 3,
ImmutableMap.of(racks.get(0), 3));
Expand All @@ -197,7 +197,7 @@ public void testReplicasToFixMisreplicationWithThreeMisreplication() {
.map(list::get).collect(Collectors.toList());
//Creating Replicas without replica Index
List<ContainerReplica> replicas = HddsTestUtils
.getReplicas(new ContainerID(1), CLOSED, 0, replicaDns);
.getReplicas(ContainerID.valueOf(1), CLOSED, 0, replicaDns);
testReplicasToFixMisreplication(replicas, dummyPlacementPolicy, 3,
ImmutableMap.of(racks.get(0), 2, racks.get(3), 1));
}
Expand All @@ -220,7 +220,7 @@ public void testReplicasToFixMisreplicationWithThreeMisreplication() {
.map(list::get).collect(Collectors.toList());
//Creating Replicas without replica Index for replicas < number of racks
List<ContainerReplica> replicas = HddsTestUtils
.getReplicas(new ContainerID(1), CLOSED, 0, replicaDns);
.getReplicas(ContainerID.valueOf(1), CLOSED, 0, replicaDns);
testReplicasToFixMisreplication(replicas, dummyPlacementPolicy, 2,
ImmutableMap.of(racks.get(0), 1, racks.get(3), 1));
}
Expand All @@ -243,7 +243,7 @@ public void testReplicasToFixMisreplicationWithThreeMisreplication() {
.map(list::get).collect(Collectors.toList());
//Creating Replicas without replica Index for replicas >number of racks
List<ContainerReplica> replicas = HddsTestUtils
.getReplicas(new ContainerID(1), CLOSED, 0, replicaDns);
.getReplicas(ContainerID.valueOf(1), CLOSED, 0, replicaDns);
testReplicasToFixMisreplication(replicas, dummyPlacementPolicy, 2,
ImmutableMap.of(racks.get(0), 1, racks.get(3), 1));
}
Expand All @@ -257,7 +257,7 @@ public void testReplicasToFixMisreplicationMaxReplicaPerRack() {
List<DatanodeDetails> replicaDns = Stream.of(0, 2, 4, 6, 8)
.map(list::get).collect(Collectors.toList());
List<ContainerReplica> replicas =
HddsTestUtils.getReplicasWithReplicaIndex(new ContainerID(1),
HddsTestUtils.getReplicasWithReplicaIndex(ContainerID.valueOf(1),
CLOSED, 0, 0, 0, replicaDns);
testReplicasToFixMisreplication(replicas, dummyPlacementPolicy, 2,
ImmutableMap.of(racks.get(0), 2));
Expand All @@ -273,7 +273,7 @@ public void testReplicasToFixMisreplicationMaxReplicaPerRack() {
List<DatanodeDetails> replicaDns = Stream.of(0, 2, 4, 6, 8)
.map(list::get).collect(Collectors.toList());
List<ContainerReplica> replicas =
HddsTestUtils.getReplicasWithReplicaIndex(new ContainerID(1),
HddsTestUtils.getReplicasWithReplicaIndex(ContainerID.valueOf(1),
CLOSED, 0, 0, 0, replicaDns);
Map<ContainerReplica, Boolean> replicaMap = replicas.stream().distinct()
.collect(Collectors.toMap(Function.identity(), r -> false));
Expand All @@ -292,7 +292,7 @@ public void testReplicasWithoutMisreplication() {
List<DatanodeDetails> replicaDns = Stream.of(0, 1, 2, 3, 4)
.map(list::get).collect(Collectors.toList());
Map<ContainerReplica, Boolean> replicas =
HddsTestUtils.getReplicasWithReplicaIndex(new ContainerID(1),
HddsTestUtils.getReplicasWithReplicaIndex(ContainerID.valueOf(1),
CLOSED, 0, 0, 0, replicaDns)
.stream()
.collect(Collectors.toMap(Function.identity(), r -> true));
Expand All @@ -308,9 +308,9 @@ public void testReplicasToRemoveWithOneOverreplication() {
List<DatanodeDetails> list = nodeManager.getAllNodes();
Set<ContainerReplica> replicas = Sets.newHashSet(
HddsTestUtils.getReplicasWithReplicaIndex(
new ContainerID(1), CLOSED, 0, 0, 0, list.subList(1, 6)));
ContainerID.valueOf(1), CLOSED, 0, 0, 0, list.subList(1, 6)));
ContainerReplica replica = ContainerReplica.newBuilder()
.setContainerID(new ContainerID(1))
.setContainerID(ContainerID.valueOf(1))
.setContainerState(CLOSED)
.setReplicaIndex(1)
.setDatanodeDetails(list.get(7)).build();
Expand All @@ -330,11 +330,11 @@ public void testReplicasToRemoveWithTwoOverreplication() {

Set<ContainerReplica> replicas = Sets.newHashSet(
HddsTestUtils.getReplicasWithReplicaIndex(
new ContainerID(1), CLOSED, 0, 0, 0, list.subList(1, 6)));
ContainerID.valueOf(1), CLOSED, 0, 0, 0, list.subList(1, 6)));

Set<ContainerReplica> replicasToBeRemoved = Sets.newHashSet(
HddsTestUtils.getReplicasWithReplicaIndex(
new ContainerID(1), CLOSED, 0, 0, 0, list.subList(7, 9)));
ContainerID.valueOf(1), CLOSED, 0, 0, 0, list.subList(7, 9)));
replicas.addAll(replicasToBeRemoved);

Set<ContainerReplica> replicasToRemove = dummyPlacementPolicy
Expand All @@ -351,14 +351,14 @@ public void testReplicasToRemoveWith2CountPerUniqueReplica() {

Set<ContainerReplica> replicas = Sets.newHashSet(
HddsTestUtils.getReplicasWithReplicaIndex(
new ContainerID(1), CLOSED, 0, 0, 0, list.subList(0, 3)));
ContainerID.valueOf(1), CLOSED, 0, 0, 0, list.subList(0, 3)));
replicas.addAll(HddsTestUtils.getReplicasWithReplicaIndex(
new ContainerID(1), CLOSED, 0, 0, 0, list.subList(3, 6)));
ContainerID.valueOf(1), CLOSED, 0, 0, 0, list.subList(3, 6)));
Set<ContainerReplica> replicasToBeRemoved = Sets.newHashSet(
HddsTestUtils.getReplicaBuilder(new ContainerID(1), CLOSED, 0, 0, 0,
HddsTestUtils.getReplicaBuilder(ContainerID.valueOf(1), CLOSED, 0, 0, 0,
list.get(7).getUuid(), list.get(7))
.setReplicaIndex(1).build(),
HddsTestUtils.getReplicaBuilder(new ContainerID(1), CLOSED, 0, 0, 0,
HddsTestUtils.getReplicaBuilder(ContainerID.valueOf(1), CLOSED, 0, 0, 0,
list.get(8).getUuid(), list.get(8)).setReplicaIndex(1)
.build());
replicas.addAll(replicasToBeRemoved);
Expand All @@ -376,7 +376,7 @@ public void testReplicasToRemoveWithoutReplicaIndex() {
List<DatanodeDetails> list = nodeManager.getAllNodes();

Set<ContainerReplica> replicas = Sets.newHashSet(HddsTestUtils.getReplicas(
new ContainerID(1), CLOSED, 0, list.subList(0, 5)));
ContainerID.valueOf(1), CLOSED, 0, list.subList(0, 5)));

Set<ContainerReplica> replicasToRemove = dummyPlacementPolicy
.replicasToRemoveToFixOverreplication(replicas, 3);
Expand All @@ -397,16 +397,16 @@ public void testReplicasToRemoveWithOverreplicationWithinSameRack() {

Set<ContainerReplica> replicas = Sets.newHashSet(
HddsTestUtils.getReplicasWithReplicaIndex(
new ContainerID(1), CLOSED, 0, 0, 0, list.subList(1, 6)));
ContainerID.valueOf(1), CLOSED, 0, 0, 0, list.subList(1, 6)));

ContainerReplica replica1 = ContainerReplica.newBuilder()
.setContainerID(new ContainerID(1))
.setContainerID(ContainerID.valueOf(1))
.setContainerState(CLOSED)
.setReplicaIndex(1)
.setDatanodeDetails(list.get(6)).build();
replicas.add(replica1);
ContainerReplica replica2 = ContainerReplica.newBuilder()
.setContainerID(new ContainerID(1))
.setContainerID(ContainerID.valueOf(1))
.setContainerState(CLOSED)
.setReplicaIndex(1)
.setDatanodeDetails(list.get(0)).build();
Expand Down Expand Up @@ -435,7 +435,7 @@ public void testReplicasToRemoveWithNoOverreplication() {
List<DatanodeDetails> list = nodeManager.getAllNodes();
Set<ContainerReplica> replicas = Sets.newHashSet(
HddsTestUtils.getReplicasWithReplicaIndex(
new ContainerID(1), CLOSED, 0, 0, 0, list.subList(1, 6)));
ContainerID.valueOf(1), CLOSED, 0, 0, 0, list.subList(1, 6)));

Set<ContainerReplica> replicasToRemove = dummyPlacementPolicy
.replicasToRemoveToFixOverreplication(replicas, 1);
Expand Down
Loading