devmadhuu commented on code in PR #11594:
URL: https://github.com/apache/ozone/pull/11594#discussion_r4422914324
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconStorageContainerSyncHelper.java:
##########
@@ -427,51 +429,50 @@ private boolean syncDeletedContainers() {
}
/**
- * Copies SCM's suppressed flag to the containers Recon already has. The
- * passes above compare only the lifecycle state, and Recon's health sweep
- * skips suppressed containers. SCM is asked only for the suppressed
- * containers, which are normally few.
+ * Copies SCM's suppressed flag to the containers Recon already has. Fetching
+ * explicit IDs avoids SCM's full container scan for each filtered list page.
*
* @return {@code true} if all RPC calls completed without error
*/
private boolean syncSuppressedFlags() {
try {
- Set<ContainerID> suppressedInScm = new HashSet<>();
- // A ContainerInfo is smaller than a ContainerWithPipeline, so that page
size is safe here.
- int batchSize = safeContainerWithPipelineBatchSize(Integer.MAX_VALUE);
+ int batchSize = safeContainerWithPipelineBatchSize((int)
getContainerCountPerCall(Integer.MAX_VALUE));
ContainerID start = ContainerID.valueOf(1);
- boolean more = true;
- while (more) {
- List<ContainerInfo> page =
scmServiceProvider.getListOfSuppressedContainerInfos(start, batchSize);
- more = !page.isEmpty();
- for (ContainerInfo info : page) {
- if (!info.isSuppressed()) {
- // The filter came with the flag, so an SCM that ignores it has no
suppressed container.
- LOG.debug("SCM listed unsuppressed container {}, so it has no
suppressed containers.", info.containerID());
- more = false;
- break;
- }
- suppressedInScm.add(info.containerID());
+ int updated = 0;
+ while (true) {
+ List<ContainerInfo> containers = containerManager.getContainers(start,
batchSize);
+ if (containers.isEmpty()) {
+ break;
}
- if (more) {
- start = ContainerID.valueOf(page.get(page.size() -
1).getContainerID() + 1);
+ List<Long> ids = new ArrayList<>(containers.size());
+ for (ContainerInfo container : containers) {
+ ids.add(container.getContainerID());
}
- }
-
- int updated = 0;
- for (ContainerInfo container : containerManager.getContainers()) {
- boolean suppressed = suppressedInScm.contains(container.containerID());
- if (container.isSuppressed() != suppressed) {
+ Map<ContainerID, ContainerInfo> scmContainers = new HashMap<>();
+ for (ContainerWithPipeline container :
scmServiceProvider.getExistContainerWithPipelinesInBatch(ids)) {
Review Comment:
Though we are now retrieving in batches, so few concerns still there:
1. First of all, this whole `syncSuppressedFlags` creating additional data
structure list of Ids which is not needed. Can we use
[this](https://github.com/apache/ozone/blob/94eacbd52ea96196b58f83a4b5d4c678994d67df/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java#L137)
?
2. Another concern, this scm API: `getExistContainerWithPipelinesInBatch` is
quite an overhead of small info Recon needs. Recon only need to know if
container is suppressed or not and this is returning Pipeline meta also over
the wire which is just a throwaway for Recon, also SCM has to create read
pipeline for all CLOSED containers on the fly and then serializes the
container and pipeline into the response, send both to Recon.That metadata
construction and transfer are unnecessary for reading a suppression flag. So I
would recommend to do this optimization either in separate follow up PR, create
JIRA and link it here or do it this PR if change is not big. Mainly we need to
have new API at SCM side which returns only relevant info.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/scm/ReconStorageContainerSyncHelper.java:
##########
@@ -427,51 +429,50 @@ private boolean syncDeletedContainers() {
}
/**
- * Copies SCM's suppressed flag to the containers Recon already has. The
- * passes above compare only the lifecycle state, and Recon's health sweep
- * skips suppressed containers. SCM is asked only for the suppressed
- * containers, which are normally few.
+ * Copies SCM's suppressed flag to the containers Recon already has. Fetching
+ * explicit IDs avoids SCM's full container scan for each filtered list page.
*
* @return {@code true} if all RPC calls completed without error
*/
private boolean syncSuppressedFlags() {
try {
- Set<ContainerID> suppressedInScm = new HashSet<>();
- // A ContainerInfo is smaller than a ContainerWithPipeline, so that page
size is safe here.
- int batchSize = safeContainerWithPipelineBatchSize(Integer.MAX_VALUE);
+ int batchSize = safeContainerWithPipelineBatchSize((int)
getContainerCountPerCall(Integer.MAX_VALUE));
ContainerID start = ContainerID.valueOf(1);
- boolean more = true;
- while (more) {
- List<ContainerInfo> page =
scmServiceProvider.getListOfSuppressedContainerInfos(start, batchSize);
- more = !page.isEmpty();
- for (ContainerInfo info : page) {
- if (!info.isSuppressed()) {
- // The filter came with the flag, so an SCM that ignores it has no
suppressed container.
- LOG.debug("SCM listed unsuppressed container {}, so it has no
suppressed containers.", info.containerID());
- more = false;
- break;
- }
- suppressedInScm.add(info.containerID());
+ int updated = 0;
+ while (true) {
+ List<ContainerInfo> containers = containerManager.getContainers(start,
batchSize);
+ if (containers.isEmpty()) {
+ break;
}
- if (more) {
- start = ContainerID.valueOf(page.get(page.size() -
1).getContainerID() + 1);
+ List<Long> ids = new ArrayList<>(containers.size());
+ for (ContainerInfo container : containers) {
+ ids.add(container.getContainerID());
}
- }
-
- int updated = 0;
- for (ContainerInfo container : containerManager.getContainers()) {
- boolean suppressed = suppressedInScm.contains(container.containerID());
- if (container.isSuppressed() != suppressed) {
+ Map<ContainerID, ContainerInfo> scmContainers = new HashMap<>();
+ for (ContainerWithPipeline container :
scmServiceProvider.getExistContainerWithPipelinesInBatch(ids)) {
+ ContainerInfo info = container.getContainerInfo();
+ scmContainers.put(info.containerID(), info);
+ }
+ for (ContainerInfo container : containers) {
+ ContainerID id = container.containerID();
try {
- containerManager.updateContainerInfo(container.containerID(),
-
container.getProtobuf().toBuilder().setSuppressed(suppressed).build());
- updated++;
+ ContainerInfo scmContainer = scmContainers.get(id);
+ if (scmContainer == null) {
+ // The batch RPC omits containers whose pipeline cannot be
resolved.
+ scmContainer = scmServiceProvider.getContainerInfo(id.getId());
Review Comment:
This will be an additional RPC call for every omitted container, just
because pipeline could not be resolved which is not needed by Recon here ?
--
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]
