[ 
https://issues.apache.org/jira/browse/HDFS-12196?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16122449#comment-16122449
 ] 

Anu Engineer commented on HDFS-12196:
-------------------------------------

[~cheersyang] , Thanks for updating the patch. It looks very good. I am almost 
a +1 for this change. I had some minor questions/comments. 

h6. One Design question
Should we allow each background task to create its own thread pool or just have 
a common thread pool?  
In that case, we will need to have a single service called BackgroundService -- 
and all other jobs like "ContainerRecyclingService" will become a task, say 
"ContainerRecyclingTask".
In other words, I am saying that we can just queue "ContainerRecyclingTask" 
directly to a background service without having individual job execution pools. 
Just one single common pool for all background jobs.
Just wondering if that is an interface we want to offer. I do see the down 
side, we can have a task monopolize the thread pool. If you are free ping me in 
slack or we can have an offline chat about this.

I am also open to checking in this architecture and may be refining it later. 
h6. some code comments

* {{BackgroundService.java}}
bq. threadFactory = r -> new Thread(threadGroup, r);
Generally we expect the background service to be a
deamon thread. Would you like to replace this with
something like 
{code}
new ThreadFactoryBuilder().setDaemon(true)
                .setNameFormat( threadName + "#%d")
                .build());
{code}
Where threadName is an argument to the ctor ? 

* {{BackgroundService.java}}
Question : Why do we need the testing flag and testing Thread?

* {{BackgroundTaskQueue.java}}
Is not thread safe, is it by design? Just wondering if it is possible for 2 
different threads to call into this concurrently. From a quick code reading, I 
am not able to see it, may we can just add a comment to the class.

*  {{ContainerRecyclingService}}
Should we create a new package called *background* tasks under  
{{org.apache.hadoop.ozone.container.common.statemachine}}
I am presuming we will need much more tasks like this in future.


Ps. Sorry for the delay in code review, I was focused on the pluggable pipeline 
patch.

> Ozone: DeleteKey-2: Implement container recycling service to delete stale 
> blocks at background
> ----------------------------------------------------------------------------------------------
>
>                 Key: HDFS-12196
>                 URL: https://issues.apache.org/jira/browse/HDFS-12196
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>          Components: ozone
>            Reporter: Weiwei Yang
>            Assignee: Weiwei Yang
>         Attachments: HDFS-12196-HDFS-7240.001.patch, 
> HDFS-12196-HDFS-7240.002.patch, HDFS-12196-HDFS-7240.003.patch, 
> HDFS-12196-HDFS-7240.004.patch
>
>
> Implement a recycling service running on datanode to delete stale blocks.  
> The recycling service scans staled blocks for each container and delete 
> chunks and references periodically.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org

Reply via email to