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-12012) Improve cancellation for the scan RPCs
Date Fri, 19 Dec 2014 23:58:14 GMT

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

stack commented on HBASE-12012:

bq. As I said, it's my opinion. You have a different opinion. That's all.

I wasn't expressing opinion.  I was just asking a question why -- thought you'd have a reason
-- and trying to help by making a suggestion. Lets avoid 'opinion'.

bq. I preferred the single 'l' for obvious reasons.

I don't get why it is obvious.

I was just looking at your patch, not wikipedia on the english language.  Usually folks aim
to be consistent. In the patch,  there is already a method that does is cancelled and it is
spelled ResultBoundedCompletionService #isCancelled. I thought you'd want to align. The old
java Cancellable Interface that I quoted you also spells it so (git blame says it was named
neither by an Englishman nor an American, but by a Frenchman)

(Not of your making -- you are just moving a class -- but the patch has this in it:

90	    public boolean isCancelled() {
91	      return canceled;

... which is not right).

bq, Sure. The startCancel was already there and I kept the same name (grep for startCancel
in the patch and you will know what I mean)

That makes sense...especially as it seems to have bubbled up from pb RpcController.

bq. This patch has been tested manually. It mainly brings the Scan cancellation at par with
how Get cancellation works - not much net new functionality.

I am suggesting you add a test so we do not regress on this new facility.  The rpc chassis
is about to be swapped out.  See HBASE-12684.  It would be a shame if we dropped your work
on the floor by mistake.

bq. If you absolutely want me to change this to a Decoration, let me know.

It is not about what I want. Was just making a suggestion.  Just trying to help out. 


class ReplicaRegionServerCallable extends RegionServerCallable<Result> implements Cancellable

seems cleaner than this:

class ReplicaRegionServerCallable extends RegionServerCallable<Result> implements CancellableCallable<Result>

Hanging a Stoppable or Abortable Decoration on an operation is how we do it elsewhere in the
codebase.  Cancellable just seemed to fit that mold.

Should RegionServerCallable ever NOT be cancellable?

To land the patch, I'd suggest you change isCancelable in your Interface -- or change the
Frenchmans' work to have one el only -- and explain how you did the manual testing so that
another coming after can try the same to make sure they have not broken this functionality
(though a test would be better -- are there tests for cancelling callables elsewhere?  If
so, would they cover?).

> Improve cancellation for the scan RPCs
> --------------------------------------
>                 Key: HBASE-12012
>                 URL: https://issues.apache.org/jira/browse/HBASE-12012
>             Project: HBase
>          Issue Type: Sub-task
>            Reporter: Devaraj Das
>            Assignee: Devaraj Das
>             Fix For: 2.0.0, 1.1.0
>         Attachments: 12012-1.txt, 12012-2.txt, 12012-3.txt, 12012-4.txt
> Similar to HBASE-11564 but for scans.

This message was sent by Atlassian JIRA

View raw message