Skip to content

Commit

Permalink
HDDS-6334. Remove ContainerID to Proto to ContainerID conversion in C…
Browse files Browse the repository at this point in the history 
…ontainerStateManagerImpl (#3110)
  • Loading branch information
@sodonnel
sodonnel committed Feb 24, 2022
1 parent 7310491 commit 93631a1
Show file tree
Hide file tree
Showing 8 changed files with 140 additions and 161 deletions.
45 changes: 14 additions & 31 deletions ...s/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java
View file
Expand Up @@ -143,7 +143,7 @@ public void reinitialize(Table<ContainerID, ContainerInfo> containerStore)
public ContainerInfo getContainer(final ContainerID id)
throws ContainerNotFoundException {
return Optional.ofNullable(containerStateManager
.getContainer(id.getProtobuf()))
.getContainer(id))
.orElseThrow(() -> new ContainerNotFoundException("ID " + id));
}

Expand All @@ -162,7 +162,6 @@ public List<ContainerInfo> getContainers(final ContainerID startID,
try {
containers = containersIds.stream()
.filter(id -> id.getId() >= start).limit(count)
.map(ContainerID::getProtobuf)
.map(containerStateManager::getContainer)
.collect(Collectors.toList());
} finally {
Expand All @@ -177,7 +176,6 @@ public List<ContainerInfo> getContainers(final LifeCycleState state) {
lock.lock();
try {
containers = containerStateManager.getContainerIDs(state).stream()
.map(ContainerID::getProtobuf)
.map(containerStateManager::getContainer)
.filter(Objects::nonNull).collect(Collectors.toList());
} finally {
Expand Down Expand Up @@ -278,18 +276,18 @@ private ContainerInfo allocateContainer(final Pipeline pipeline,
.build();
containerStateManager.addContainer(containerInfo);
scmContainerManagerMetrics.incNumSuccessfulCreateContainers();
return containerStateManager.getContainer(containerID.getProtobuf());
return containerStateManager.getContainer(containerID);
}

@Override
public void updateContainerState(final ContainerID id,
public void updateContainerState(final ContainerID cid,
final LifeCycleEvent event)
throws IOException, InvalidStateTransitionException {
final HddsProtos.ContainerID cid = id.getProtobuf();
HddsProtos.ContainerID protoId = cid.getProtobuf();
lock.lock();
try {
if (containerExist(cid)) {
containerStateManager.updateContainerState(cid, event);
containerStateManager.updateContainerState(protoId, event);
} else {
throwContainerNotFoundException(cid);
}
Expand All @@ -302,15 +300,14 @@ public void updateContainerState(final ContainerID id,
public Set<ContainerReplica> getContainerReplicas(final ContainerID id)
throws ContainerNotFoundException {
return Optional.ofNullable(containerStateManager
.getContainerReplicas(id.getProtobuf()))
.getContainerReplicas(id))
.orElseThrow(() -> new ContainerNotFoundException("ID " + id));
}

@Override
public void updateContainerReplica(final ContainerID id,
public void updateContainerReplica(final ContainerID cid,
final ContainerReplica replica)
throws ContainerNotFoundException {
final HddsProtos.ContainerID cid = id.getProtobuf();
if (containerExist(cid)) {
containerStateManager.updateContainerReplica(cid, replica);
} else {
Expand All @@ -319,10 +316,9 @@ public void updateContainerReplica(final ContainerID id,
}

@Override
public void removeContainerReplica(final ContainerID id,
public void removeContainerReplica(final ContainerID cid,
final ContainerReplica replica)
throws ContainerNotFoundException, ContainerReplicaNotFoundException {
final HddsProtos.ContainerID cid = id.getProtobuf();
if (containerExist(cid)) {
containerStateManager.removeContainerReplica(cid, replica);
} else {
Expand Down Expand Up @@ -414,13 +410,13 @@ public void notifyContainerReportProcessing(final boolean isFullReport,
}

@Override
public void deleteContainer(final ContainerID id)
public void deleteContainer(final ContainerID cid)
throws IOException {
final HddsProtos.ContainerID cid = id.getProtobuf();
HddsProtos.ContainerID protoId = cid.getProtobuf();
lock.lock();
try {
if (containerExist(cid)) {
containerStateManager.removeContainer(cid);
containerStateManager.removeContainer(protoId);
scmContainerManagerMetrics.incNumSuccessfulDeleteContainers();
} else {
scmContainerManagerMetrics.incNumFailureDeleteContainers();
Expand All @@ -431,28 +427,15 @@ public void deleteContainer(final ContainerID id)
}
}

@Deprecated
private void checkIfContainerExist(final HddsProtos.ContainerID id)
throws ContainerNotFoundException {
if (!containerStateManager.contains(id)) {
throw new ContainerNotFoundException("Container with id #" +
id.getId() + " not found.");
}
}

@Override
public boolean containerExist(final ContainerID id) {
return containerExist(id.getProtobuf());
}

private boolean containerExist(final HddsProtos.ContainerID id) {
return containerStateManager.contains(id);
}

private void throwContainerNotFoundException(final HddsProtos.ContainerID id)
private void throwContainerNotFoundException(final ContainerID id)
throws ContainerNotFoundException {
throw new ContainerNotFoundException("Container with id #" +
id.getId() + " not found.");
throw new ContainerNotFoundException("Container with id " +
id + " not found.");
}

@Override
Expand Down
10 changes: 5 additions & 5 deletions .../server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManager.java
View file
Expand Up @@ -100,7 +100,7 @@ public interface ContainerStateManager {
/**
*
*/
boolean contains(HddsProtos.ContainerID containerID);
boolean contains(ContainerID containerID);

/**
* Returns the ID of all the managed containers.
Expand All @@ -117,23 +117,23 @@ public interface ContainerStateManager {
/**
*
*/
ContainerInfo getContainer(HddsProtos.ContainerID id);
ContainerInfo getContainer(ContainerID id);

/**
*
*/
Set<ContainerReplica> getContainerReplicas(HddsProtos.ContainerID id);
Set<ContainerReplica> getContainerReplicas(ContainerID id);

/**
*
*/
void updateContainerReplica(HddsProtos.ContainerID id,
void updateContainerReplica(ContainerID id,
ContainerReplica replica);

/**
*
*/
void removeContainerReplica(HddsProtos.ContainerID id,
void removeContainerReplica(ContainerID id,
ContainerReplica replica);

/**
Expand Down
24 changes: 10 additions & 14 deletions ...ver-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManagerImpl.java
View file
Expand Up @@ -277,10 +277,10 @@ public Set<ContainerID> getContainerIDs(final LifeCycleState state) {
}

@Override
public ContainerInfo getContainer(final HddsProtos.ContainerID id) {
public ContainerInfo getContainer(final ContainerID id) {
lock.readLock().lock();
try {
return containers.getContainerInfo(ContainerID.getFromProtobuf(id));
return containers.getContainerInfo(id);
} finally {
lock.readLock().unlock();
}
Expand Down Expand Up @@ -326,11 +326,10 @@ public void addContainer(final ContainerInfoProto containerInfo)
}

@Override
public boolean contains(final HddsProtos.ContainerID id) {
public boolean contains(ContainerID id) {
lock.readLock().lock();
try {
// TODO: Remove the protobuf conversion after fixing ContainerStateMap.
return containers.contains(ContainerID.getFromProtobuf(id));
return containers.contains(id);
} finally {
lock.readLock().unlock();
}
Expand Down Expand Up @@ -370,35 +369,32 @@ public void updateContainerState(final HddsProtos.ContainerID containerID,


@Override
public Set<ContainerReplica> getContainerReplicas(
final HddsProtos.ContainerID id) {
public Set<ContainerReplica> getContainerReplicas(final ContainerID id) {
lock.readLock().lock();
try {
return containers.getContainerReplicas(
ContainerID.getFromProtobuf(id));
return containers.getContainerReplicas(id);
} finally {
lock.readLock().unlock();
}
}

@Override
public void updateContainerReplica(final HddsProtos.ContainerID id,
public void updateContainerReplica(final ContainerID id,
final ContainerReplica replica) {
lock.writeLock().lock();
try {
containers.updateContainerReplica(ContainerID.getFromProtobuf(id),
replica);
containers.updateContainerReplica(id, replica);
} finally {
lock.writeLock().unlock();
}
}

@Override
public void removeContainerReplica(final HddsProtos.ContainerID id,
public void removeContainerReplica(final ContainerID id,
final ContainerReplica replica) {
lock.writeLock().lock();
try {
containers.removeContainerReplica(ContainerID.getFromProtobuf(id),
containers.removeContainerReplica(id,
replica);
} finally {
lock.writeLock().unlock();
Expand Down
28 changes: 14 additions & 14 deletions ...er-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerReportHandler.java
View file
Expand Up @@ -103,13 +103,13 @@ public void setup() throws IOException, InvalidStateTransitionException {
Mockito.when(containerManager.getContainer(Mockito.any(ContainerID.class)))
.thenAnswer(invocation -> containerStateManager
.getContainer(((ContainerID)invocation
.getArguments()[0]).getProtobuf()));
.getArguments()[0])));

Mockito.when(containerManager.getContainerReplicas(
Mockito.any(ContainerID.class)))
.thenAnswer(invocation -> containerStateManager
.getContainerReplicas(((ContainerID)invocation
.getArguments()[0]).getProtobuf()));
.getArguments()[0])));

Mockito.doAnswer(invocation -> {
containerStateManager
Expand All @@ -123,15 +123,15 @@ public void setup() throws IOException, InvalidStateTransitionException {

Mockito.doAnswer(invocation -> {
containerStateManager.updateContainerReplica(
((ContainerID)invocation.getArguments()[0]).getProtobuf(),
((ContainerID)invocation.getArguments()[0]),
(ContainerReplica) invocation.getArguments()[1]);
return null;
}).when(containerManager).updateContainerReplica(
Mockito.any(ContainerID.class), Mockito.any(ContainerReplica.class));

Mockito.doAnswer(invocation -> {
containerStateManager.removeContainerReplica(
((ContainerID)invocation.getArguments()[0]).getProtobuf(),
((ContainerID)invocation.getArguments()[0]),
(ContainerReplica) invocation.getArguments()[1]);
return null;
}).when(containerManager).removeContainerReplica(
Expand Down Expand Up @@ -177,13 +177,13 @@ public void testUnderReplicatedContainer()
ContainerReplicaProto.State.CLOSED,
datanodeOne, datanodeTwo, datanodeThree)
.forEach(r -> containerStateManager.updateContainerReplica(
containerOne.containerID().getProtobuf(), r));
containerOne.containerID(), r));

getReplicas(containerTwo.containerID(),
ContainerReplicaProto.State.CLOSED,
datanodeOne, datanodeTwo, datanodeThree)
.forEach(r -> containerStateManager.updateContainerReplica(
containerTwo.containerID().getProtobuf(), r));
containerTwo.containerID(), r));


// SCM expects both containerOne and containerTwo to be in all the three
Expand Down Expand Up @@ -236,13 +236,13 @@ public void testOverReplicatedContainer() throws NodeNotFoundException,
ContainerReplicaProto.State.CLOSED,
datanodeOne, datanodeTwo, datanodeThree)
.forEach(r -> containerStateManager.updateContainerReplica(
containerOne.containerID().getProtobuf(), r));
containerOne.containerID(), r));

getReplicas(containerTwo.containerID(),
ContainerReplicaProto.State.CLOSED,
datanodeOne, datanodeTwo, datanodeThree)
.forEach(r -> containerStateManager.updateContainerReplica(
containerTwo.containerID().getProtobuf(), r));
containerTwo.containerID(), r));


// SCM expects both containerOne and containerTwo to be in all the three
Expand Down Expand Up @@ -315,11 +315,11 @@ public void testClosingToClosed() throws NodeNotFoundException, IOException {

containerOneReplicas.forEach(r ->
containerStateManager.updateContainerReplica(
containerTwo.containerID().getProtobuf(), r));
containerTwo.containerID(), r));

containerTwoReplicas.forEach(r ->
containerStateManager.updateContainerReplica(
containerTwo.containerID().getProtobuf(), r));
containerTwo.containerID(), r));


final ContainerReportsProto containerReport = getContainerReportsProto(
Expand Down Expand Up @@ -383,11 +383,11 @@ public void testClosingToQuasiClosed()

containerOneReplicas.forEach(r ->
containerStateManager.updateContainerReplica(
containerTwo.containerID().getProtobuf(), r));
containerTwo.containerID(), r));

containerTwoReplicas.forEach(r ->
containerStateManager.updateContainerReplica(
containerTwo.containerID().getProtobuf(), r));
containerTwo.containerID(), r));


final ContainerReportsProto containerReport = getContainerReportsProto(
Expand Down Expand Up @@ -454,11 +454,11 @@ public void testQuasiClosedToClosed()

containerOneReplicas.forEach(r ->
containerStateManager.updateContainerReplica(
containerTwo.containerID().getProtobuf(), r));
containerTwo.containerID(), r));

containerTwoReplicas.forEach(r ->
containerStateManager.updateContainerReplica(
containerTwo.containerID().getProtobuf(), r));
containerTwo.containerID(), r));


final ContainerReportsProto containerReport = getContainerReportsProto(
Expand Down
6 changes: 3 additions & 3 deletions ...ver-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerStateManager.java
View file
Expand Up @@ -120,7 +120,7 @@ public void checkReplicationStateOK() throws IOException {

//WHEN
Set<ContainerReplica> replicas = containerStateManager
.getContainerReplicas(c1.containerID().getProtobuf());
.getContainerReplicas(c1.containerID());

//THEN
Assert.assertEquals(3, replicas.size());
Expand All @@ -140,7 +140,7 @@ public void checkReplicationStateMissingReplica() throws IOException {

//WHEN
Set<ContainerReplica> replicas = containerStateManager
.getContainerReplicas(c1.containerID().getProtobuf());
.getContainerReplicas(c1.containerID());

Assert.assertEquals(2, replicas.size());
Assert.assertEquals(3, c1.getReplicationConfig().getRequiredNodes());
Expand All @@ -153,7 +153,7 @@ private void addReplica(ContainerInfo cont, DatanodeDetails node) {
.setDatanodeDetails(node)
.build();
containerStateManager
.updateContainerReplica(cont.containerID().getProtobuf(), replica);
.updateContainerReplica(cont.containerID(), replica);
}

private ContainerInfo allocateContainer() throws IOException {
Expand Down

0 comments on commit 93631a1

Please sign in to comment.