Mailing-List: contact dev-help@hbase.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hbase.apache.org
Received-SPF: pass (nike.apache.org: domain of mac.hadoop@gmail.com designates
 209.85.161.169 as permitted sender)
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=mime-version:in-reply-to:references:date:message-id:subject:from:to
         :content-type;
        b=OzPr/wNquNTmQ0k1Uk4IoWJSuM2+tA5z4wKVxOBUzYHfpxB3lxPC6va2woJudWr4a8
         Z4OsV5LBUyoFqVJT+vVRI9tRujMdoHCB2gKQP6YFstkEsQYxfsCwlyNQR4IU43eMHGxB
         o0YqVeOVzDGNeYhVNctdO5qREysA6w+fFUh1k=
MIME-Version: 1.0
In-Reply-To: <AANLkTim6vy19SVDmOqNaUZDFBjx979f8dWxa36L9yxO6@mail.gmail.com>
References: <AANLkTinY=N9UMsgtMPmsFaXV6+i7Q9f2rr3SPZNVOwEu@mail.gmail.com>
	<AANLkTi=DpAExUex5m8s_b5N2vwO0-93NVTSBGWSz2mXL@mail.gmail.com>
	<AANLkTim=zieQMwRFCu=ghPkhetM2c93zHWotSEEvJGj6@mail.gmail.com>
	<AANLkTinbYm013OqT40tfEwSO=HtEH9WY3S8bN6VhS6H7@mail.gmail.com>
	<AANLkTimguq=KF8kx+FcxrVux2AbpJFPPyLTe6R+FkB_V@mail.gmail.com>
	<AANLkTime5j+13OUrnJ9gLpRta1B82v8Ft+3zwGvxPE74@mail.gmail.com>
	<AANLkTim6vy19SVDmOqNaUZDFBjx979f8dWxa36L9yxO6@mail.gmail.com>
Date: Wed, 26 Jan 2011 12:34:28 +0800
Message-ID: <AANLkTik0=8w9m54Yw7BAwt81RZkpwPYefK4WtczAKWXM@mail.gmail.com>
Subject: Re: Looks like duplicate in MemoryStoreFlusher flushSomeRegions()
From: mac fang <mac.hadoop@gmail.com>
To: dev@hbase.apache.org
Content-Type: multipart/alternative; boundary=0016e642d15a419e8d049ab85964

--0016e642d15a419e8d049ab85964
Content-Type: text/plain; charset=ISO-8859-1

Oh, yes, I checked the code, the method protected CompactionRequest
addToRegionsInQueue(HRegion r, int p) {

contains the:

      if (queuedRequest == null ||
          newRequest.getPriority() < queuedRequest.getPriority()) {
        LOG.trace("Inserting region in queue. " + newRequest);
        regionsInQueue.put(r, newRequest);

So it should be OK. Thanks for remind.

regards
macf


On Wed, Jan 26, 2011 at 10:30 AM, Ryan Rawson <ryanobjc@gmail.com> wrote:

> it does seem like the call to compactionRequested calls in
>
> (Class MemStoreFlusher)
>  private synchronized void flushSomeRegions();
>
> are superfluous.  Due to the nature of the compaction algorithm this
> should not cause extra compaction, so the impact should be minimal,
> but ideally should be removed.
>
> -ryan
>
> On Tue, Jan 25, 2011 at 6:23 PM, Ryan Rawson <ryanobjc@gmail.com> wrote:
> > the call to compactionRequested() only puts the region on a queue to
> > be compacted, so if there is unintended duplication, it wont actually
> > hold anything up.
> >
> > -ryan
> >
> > On Tue, Jan 25, 2011 at 6:05 PM, mac fang <mac.hadoop@gmail.com> wrote:
> >> Guys, since the flushCache will make the write/read suspend. I am NOT
> sure
> >> if it is necessary here.
> >>
> >> On Mon, Jan 24, 2011 at 1:48 PM, mac fang <mac.hadoop@gmail.com> wrote:
> >>
> >>> Yes, I mean the server.compactSplitThread.compactionRequested(region,
> >>> getName());
> >>>
> >>> in flushRegion, it will do the
> server.compactSplitThread.compactionRequested(region,
> >>> getName());
> >>>
> >>> *seems we don't need to do it again in the following logic (can you
> guys
> >>> see the lines in bold, **!flushRegion(biggestMemStoreRegion, true) and
> *
> >>> *
> >>> *
> >>> *    for (HRegion region : regionsToCompact) {
> >>>       server.compactSplitThread.compactionRequested(region, getName());
> >>>     }*
> >>> *
> >>> *
> >>> *
> >>> *
> >>> *regards*
> >>> macf
> >>>
> >>>
> >>>     if (*!flushRegion(biggestMemStoreRegion, true)*) {
> >>>         LOG.warn("Flush failed");
> >>>         break;
> >>>       }
> >>>       regionsToCompact.add(biggestMemStoreRegion);
> >>>     }
> >>>     for (HRegion region : regionsToCompact) {
> >>>       *server.compactSplitThread.compactionRequested(region,
> getName());*
> >>>     }
> >>>
> >>> > > in flushRegion
> >>> > >
> >>> > >  private boolean flushRegion(final HRegion region, final boolean
> >>> > > emergencyFlush) {
> >>> > >    synchronized (this.regionsInQueue) {
> >>> > >      FlushQueueEntry fqe = this.regionsInQueue.remove(region);
> >>> > >      if (fqe != null && emergencyFlush) {
> >>> > >        // Need to remove from region from delay queue.  When NOT an
> >>> > >        // emergencyFlush, then item was removed via a
> flushQueue.poll.
> >>> > >        flushQueue.remove(fqe);
> >>> > >      }
> >>> > >      lock.lock();
> >>> > >    }
> >>> > >    try {
> >>> > >      if (region.flushcache()) {
> >>> > >        *server.compactSplitThread.compactionRequested(region,
> >>> getName());*
> >>> > >      }
> >>>
> >>> On Mon, Jan 24, 2011 at 6:40 AM, Ted Yu <yuzhihong@gmail.com> wrote:
> >>>
> >>>> I think he was referring to this line:
> >>>>
> >>>> server.compactSplitThread.compactionRequested(region, getName());
> >>>>
> >>>> On Sun, Jan 23, 2011 at 10:52 AM, Stack <stack@duboce.net> wrote:
> >>>>
> >>>> > Hello Mac Fang:  Which lines in the below?  Your colorizing didn't
> >>>> > come across in the mail.  Thanks, St.Ack
> >>>> >
> >>>> > On Sun, Jan 23, 2011 at 6:23 AM, mac fang <mac.hadoop@gmail.com>
> wrote:
> >>>> > > Hi, guys,
> >>>> > >
> >>>> > > see the below codes in* MemStoreFlusher.java*, i am not sure if
> those
> >>>> >  lines
> >>>> > > in orange are the same and looks like they are trying to do the
> same
> >>>> > logic.
> >>>> > > Are they redundant?
> >>>> > >
> >>>> > > regards
> >>>> > > macf
> >>>> > >
> >>>> > >    if (!flushRegion(biggestMemStoreRegion, true)) {
> >>>> > >        LOG.warn("Flush failed");
> >>>> > >        break;
> >>>> > >      }
> >>>> > >      regionsToCompact.add(biggestMemStoreRegion);
> >>>> > >    }
> >>>> > >    for (HRegion region : regionsToCompact) {
> >>>> > >      server.compactSplitThread.compactionRequested(region,
> getName());
> >>>> > >    }
> >>>> > >
> >>>> > > in flushRegion
> >>>> > >
> >>>> > >  private boolean flushRegion(final HRegion region, final boolean
> >>>> > > emergencyFlush) {
> >>>> > >    synchronized (this.regionsInQueue) {
> >>>> > >      FlushQueueEntry fqe = this.regionsInQueue.remove(region);
> >>>> > >      if (fqe != null && emergencyFlush) {
> >>>> > >        // Need to remove from region from delay queue.  When NOT
> an
> >>>> > >        // emergencyFlush, then item was removed via a
> flushQueue.poll.
> >>>> > >        flushQueue.remove(fqe);
> >>>> > >      }
> >>>> > >      lock.lock();
> >>>> > >    }
> >>>> > >    try {
> >>>> > >      if (region.flushcache()) {
> >>>> > >        server.compactSplitThread.compactionRequested(region,
> >>>> getName());
> >>>> > >      }
> >>>> > >
> >>>> >
> >>>>
> >>>
> >>>
> >>
> >
>

--0016e642d15a419e8d049ab85964--