hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "stack (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-6778) Deprecate Chore; its a thread per task when we should have one thread to do all tasks
Date Tue, 20 Jan 2015 21:43:35 GMT

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

stack commented on HBASE-6778:
------------------------------

Nice writeup [~jonathan.lawlor]

ChoreService should be at the top-level I believe as is Chore (right?) It is used all over
the code base.

Before final patch, backfill doc.  Above outline would be good skeleton. For example, nothing
currently on ChoreService but that is because this a WIP.

Doc on data members is good quality.

Not needed:

88	    if (chore == null) {
89	      throw new NullPointerException("chore cannot be null");
90	    }

Why not have intialdelay be Chore too rather than pass it in?  Because it a one-off?  Gets
a bit awkward when you have to say this: "The initial delay before the scheduled chore becomes
active. Measured in
109	   *          same units as chore.getTimeUnit()"  ... in other words, the initial delay
depends on Chore... bettter if if didnt'.  This is a nit.

If all methods are synchronized, why do we need concurrent hash map?

You should probably tighten up your class and not let stuff like 	  public int getNumberOfChoresMissingStartTime()
{ out to anyone but unit tests.  TMI.

Check out how threads are named in hbase. We'll usually add the server name for a prefix.
Helps when you have many of them all running in the one JVM.

This should be internal detail?

  public synchronized boolean requestCorePoolIncrease() {

Shut down the likes of t his till someone asks for it:

		218	  public void onChoreMadeStartTime(ScheduledChore chore) {
219	    // TODO: This is a placeholder for future extension... As it currently stands,

Shut down access to this?

225	  public synchronized void onChoreMissedStartTime(ScheduledChore chore) {

Internal detail?

I'd say keep the debug stuff in but just make it log at trace level (and check trace level
is set).

FYI, don't need public qualifiers when defining an Interface...   public interface ChoreServicer
{  Its always public.

Do we need to reschedule?  I don't remember this feature.  It is nice but maybe not needed?

Very nice tests.

Coming along real nice [~jonathan.lawlor]













> Deprecate Chore; its a thread per task when we should have one thread to do all tasks
> -------------------------------------------------------------------------------------
>
>                 Key: HBASE-6778
>                 URL: https://issues.apache.org/jira/browse/HBASE-6778
>             Project: HBase
>          Issue Type: Bug
>            Reporter: stack
>            Assignee: Jonathan Lawlor
>         Attachments: HBASE_6778_WIP_v1.patch, thread_dump_HMaster.local.out
>
>
> Should use something like ScheduledThreadPoolExecutor instead (Elliott said this first
I think; J-D said something similar just now).



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message