You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "Gargi-jais11 (via GitHub)" <gi...@apache.org> on 2025/03/21 08:51:54 UTC
[PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Gargi-jais11 opened a new pull request, #8127:
URL: https://github.com/apache/ozone/pull/8127
## What changes were proposed in this pull request?
If a container has push mode moving task already, there is chance that this container's replica will be deleted in future. Avoid selecting such state containers. Also avoid selecting empty container to move.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-12433
## How was this patch tested?
Added a unit test for `DefaultContainerChoosingPolicy.java` in `TestDefaultContainerChoosingPolicy` class.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #8127:
URL: https://github.com/apache/ozone/pull/8127#issuecomment-3051071453
> This would handle the edge case where a container deletion was already in progress when we acquired the lock.
@peterxcli , shouldn't a container deletion operation hold the lock before it finish the execution?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #8127:
URL: https://github.com/apache/ozone/pull/8127#discussion_r2009633945
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/ContainerChoosingPolicy.java:
##########
@@ -32,5 +33,6 @@ public interface ContainerChoosingPolicy {
* @return a Container
*/
ContainerData chooseContainer(OzoneContainer ozoneContainer,
- HddsVolume volume, Set<Long> inProgressContainerIDs);
+ HddsVolume volume, Set<Long> inProgressContainerIDs,
+ ReplicationSupervisor replicationSupervisor);
Review Comment:
ReplicationSupervisor replicationSupervisor -> Set<Long> replicationContainerIDs
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #8127:
URL: https://github.com/apache/ozone/pull/8127#discussion_r2052221550
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultContainerChoosingPolicy.java:
##########
@@ -35,13 +35,17 @@ public class DefaultContainerChoosingPolicy implements ContainerChoosingPolicy {
@Override
public ContainerData chooseContainer(OzoneContainer ozoneContainer,
- HddsVolume hddsVolume, Set<Long> inProgressContainerIDs) {
+ HddsVolume hddsVolume, Set<Long> inProgressContainerIDs,
+ Set<Long> replicationContainerIDs) {
Iterator<Container<?>> itr = ozoneContainer.getController()
.getContainers(hddsVolume);
while (itr.hasNext()) {
ContainerData containerData = itr.next().getContainerData();
- if (!inProgressContainerIDs.contains(
- containerData.getContainerID()) && containerData.isClosed()) {
+ long containerID = containerData.getContainerID();
+
+ if (!replicationContainerIDs.contains(containerID) &&
+ !inProgressContainerIDs.contains(containerID) &&
+ containerData.isClosed() && !containerData.isEmpty()) {
Review Comment:
please check isClosed and isEmpty() first.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #8127:
URL: https://github.com/apache/ozone/pull/8127#discussion_r2009649342
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/ContainerChoosingPolicy.java:
##########
@@ -32,5 +33,6 @@ public interface ContainerChoosingPolicy {
* @return a Container
*/
ContainerData chooseContainer(OzoneContainer ozoneContainer,
- HddsVolume volume, Set<Long> inProgressContainerIDs);
+ HddsVolume volume, Set<Long> inProgressContainerIDs,
+ ReplicationSupervisor replicationSupervisor);
Review Comment:
Please complete the java doc, adding the parameters, and explain why the two container IDs should be skipped.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #8127:
URL: https://github.com/apache/ozone/pull/8127#discussion_r2009632308
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java:
##########
@@ -263,15 +269,24 @@ public OzoneContainer(HddsDatanodeService hddsDatanodeService,
blockDeletingServiceWorkerSize, config,
datanodeDetails.threadNamePrefix(),
context.getParent().getReconfigurationHandler());
+ Clock clock = Clock.system(ZoneId.systemDefault());
Duration diskBalancerSvcInterval = conf.getObject(
DiskBalancerConfiguration.class).getDiskBalancerInterval();
Duration diskBalancerSvcTimeout = conf.getObject(
DiskBalancerConfiguration.class).getDiskBalancerTimeout();
+
+ replicationSupervisor = ReplicationSupervisor.newBuilder()
Review Comment:
No need to create a new ReplicationSupervisor. This newly created ReplicationSupervisor will not be used by replication.
get the ReplicationSupervisor from hddsDatanodeService.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #8127:
URL: https://github.com/apache/ozone/pull/8127#issuecomment-2871367600
> For me, this change is premature optimisation. This is not a problem we have seen and as I said earlier, this change only solves a very small percentage of the potential cases where a container will be deleted right after being moved by the disk balancer.
>
> I am still against this change being merged, unless we can see evidence of such a problem occurring frequently in practice.
Fine, I respect one's right to say no though I don't agree. Didn't expect there will so strong disagreement on this optimization.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #8127:
URL: https://github.com/apache/ozone/pull/8127#issuecomment-2742946702
Is this check really needed?
I am not too sure how the disk balancer moves a container, but it should take a lock on the container to ensure it is not changed while the container contents are being copies to the new disk. That would prevent replication from starting, as it must get a lock on the container to export the container data too.
Then when the data is copied to the new disk, I assume the disk balancer will atomically swap the in memory view of the container from old disk to new disk, before removing the data on the old disk.
Due to the locking, if an push replication is in progress, the balancer will not be able to lock and will wait, and the opposite is true too - the replication would wait on the balance task on the container.
I seems like this should all be handled by the lock on the KeyValueContainer instance?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #8127:
URL: https://github.com/apache/ozone/pull/8127#discussion_r2147030526
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java:
##########
@@ -675,6 +687,10 @@ public ReplicationServer getReplicationServer() {
return replicationServer;
}
+ public ReplicationSupervisor getReplicationSupervisor() {
Review Comment:
This can be removed.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #8127:
URL: https://github.com/apache/ozone/pull/8127#issuecomment-2746827590
> > has the chance to be deleted later for reasons such as mis-replicated.
>
> Any container has this chance. Even if a container was push replicated due to mis-replication, there is only a 1 in 3 chance that it will be the one that gets deleted. In fact, the disk balancer could be running on another node and that is the one that is selected for delete, and there is no check for that.
>
> Normally, there will be no replication going on. A cluster should not be continuously replicating. Therefore the chances of the disk balancer moving a container that is just about to be deleted is likely very small, and if it does, its just a small amount of wasted work.
>
> While this change is not over large, it adds complexity in both the implementation and tests which must be understood now and in the future and probably isn't worth it.
I don't favor complexity too. But the IO resources on datanode are very precious, data read, write, scan, replication, raft, rocksdb. So it naturally come with idea to save potential wasted IO resources, which I believe is not hard to understand. Implementation wise, comment to explain the motivation is required, others we will see.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #8127:
URL: https://github.com/apache/ozone/pull/8127#issuecomment-2743008869
> Is this check really needed?
>
> I am not too sure how the disk balancer moves a container, but it should take a lock on the container to ensure it is not changed while the container contents are being copies to the new disk. That would prevent replication from starting, as it must get a lock on the container to export the container data too.
>
> Then when the data is copied to the new disk, I assume the disk balancer will atomically swap the in memory view of the container from old disk to new disk, before removing the data on the old disk.
>
> Due to the locking, if an push replication is in progress, the balancer will not be able to lock and will wait, and the opposite is true too - the replication would wait on the balance task on the container.
>
> I seems like this should all be handled by the lock on the KeyValueContainer instance?
The container which is replicated to other datanode, has the chance to be deleted later for reasons such as mis-replicated. So the task is to avoid selecting those containers to move at the current iterating, not to avoid the contention between two types of work.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #8127:
URL: https://github.com/apache/ozone/pull/8127#issuecomment-2824846168
For me, this change is premature optimisation. This is not a problem we have seen and as I said earlier, this change only solves a very small percentage of the potential cases where a container will be deleted right after being moved by the disk balancer.
I am still against this change being merged, unless we can see evidence of such a problem occurring frequently in practice.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #8127:
URL: https://github.com/apache/ozone/pull/8127#discussion_r2052220353
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/ContainerChoosingPolicy.java:
##########
@@ -28,9 +28,14 @@
public interface ContainerChoosingPolicy {
/**
* Choose a container for balancing.
- *
+ * @param inProgressContainerIDs containerIDs present in this set should be
Review Comment:
@Gargi-jais11 , please add @param for ozoneContainer and volume too, to complete the jave doc for this function.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #8127:
URL: https://github.com/apache/ozone/pull/8127#issuecomment-2743093699
> has the chance to be deleted later for reasons such as mis-replicated.
Any container has this chance. Even if a container was push replicated due to mis-replication, there is only a 1 in 3 chance that it will be the one that gets deleted. In fact, the disk balancer could be running on another node and that is the one that is selected for delete, and there is no check for that.
Normally, there will be no replication going on. A cluster should not be continuously replicating. Therefore the chances of the disk balancer moving a container that is just about to be deleted is likely very small, and if it does, its just a small amount of wasted work.
While this change is not over large, it adds complexity in both the implementation and tests which must be understood now and in the future and probably isn't worth it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on PR #8127:
URL: https://github.com/apache/ozone/pull/8127#issuecomment-2747728636
> So it naturally come with idea to save potential wasted IO resources, which I believe is not hard to understand
Complexity is built up with lots of "things like this" over time. This change may be somewhat simple, but it does increase coupling in the system, as now replication supervisor is depended on by the Disk Balancer, where it otherwise would not be.
I don't think we should make this change. The chances of this saving a tiny bit of I/O on a cluster once or twice isn't worth the additional code.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #8127:
URL: https://github.com/apache/ozone/pull/8127#discussion_r2052220353
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/ContainerChoosingPolicy.java:
##########
@@ -28,9 +28,14 @@
public interface ContainerChoosingPolicy {
/**
* Choose a container for balancing.
- *
+ * @param inProgressContainerIDs containerIDs present in this set should be
Review Comment:
please add @param for ozoneContainer and volume too, to complete the jave doc for this function.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #8127:
URL: https://github.com/apache/ozone/pull/8127#discussion_r2052238140
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/diskbalancer/TestDiskBalancerService.java:
##########
@@ -252,6 +253,8 @@ private OzoneContainer mockDependencies(ContainerSet containerSet,
when(dispatcher.getHandler(any())).thenReturn(keyValueHandler);
when(ozoneContainer.getVolumeSet()).thenReturn(volumeSet);
when(ozoneContainer.getController()).thenReturn(controller);
+ ReplicationSupervisor replicationSupervisor = mock(ReplicationSupervisor.class);
Review Comment:
Is this mock replicationSupervisor for ozoneContainer needed?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #8127:
URL: https://github.com/apache/ozone/pull/8127#issuecomment-2808818760
> > So it naturally come with idea to save potential wasted IO resources, which I believe is not hard to understand
>
> Complexity is built up with lots of "things like this" over time. This change may be somewhat simple, but it does increase coupling in the system, as now replication supervisor is depended on by the Disk Balancer, where it otherwise would not be.
>
> I don't think we should make this change. The chances of this saving a tiny bit of I/O on a cluster once or twice isn't worth the additional code.
I would say optimization and complexity most time come together. But this optimization is a very low hanging fruit, it doesn't require complex logic. And in a heavy workload datanode environment, I believe it can avoid things getting worse. I would prefer to keep it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi closed pull request #8127: HDDS-12433. [DiskBalancer] Avoid selecting container which has container balancer or replication task already
URL: https://github.com/apache/ozone/pull/8127
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org