Mailing-List: contact issues-help@hbase.apache.org; run by ezmlm
Precedence: bulk
Date: Tue, 14 Jan 2014 06:35:54 +0000 (UTC)
From: "stack (JIRA)" <jira@apache.org>
To: issues@hbase.apache.org
Message-ID: <JIRA.12688432.1389509960813.92081.1389681354982@arcas>
In-Reply-To: <JIRA.12688432.1389509960813@arcas>
References: <JIRA.12688432.1389509960813@arcas>
Subject: [jira] [Commented] (HBASE-10322) Strip tags from KV while sending
 back to client on reads
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


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

stack commented on HBASE-10322:
-------------------------------

Let me continue the review Anoop (sorry took a while to get back here):

{code}
-        ByteBuffer cellBlock = ipcUtil.buildCellBlock(this.codec, this.compressor, call.cells);
+        ByteBuffer cellBlock = ipcUtil
+            .buildCellBlock(this.codec, this.compressor, call.cells, true);
{code}
The above is in RpcClient.  Can the Codec be a client codec rather than pass a 'true' for client context?  (Just asking....)

This looks good:

{code}
+    if (RequestContext.isSuperUserRequest()) {
+      kvbuilder.setTags(ZeroCopyLiteralByteString.wrap(kv.getTagsArray(), kv.getTagsOffset(),
+          kv.getTagsLength()));
+    }
{code}

.... as long as that call to isSuperUserRequest is cheap!!!!

Is this call inexpensive?

+    if (cell.hasTags()) {


What is this?

+  public static long oswrite(final KeyValue kv, final OutputStream out, final boolean withTags)

You pass a flag if you want tags written?  This saves a KV copy I'd imagine? If so, that is good.

Now we never write tags in CellCodec?

-      // Write tags
-      write(cell.getTagsArray(), cell.getTagsOffset(), cell.getTagsLength());
+      // TODO writing tags can be implemented once we do connection negotiation work
       // MvccVersion


How are we going to do the following in a backward compatible way?

+      // TODO writing tags can be implemented once we do connection negotiation work

Are we going to up the rpc version number?

It is odd that CellCodec Interface knows about 'client':

+  public Decoder getClientDecoder(InputStream is) {

It shouldn't have to.

Ditto for getServerDecoder.

Codec should know nothing about client and server.

The below seems wrong to me:

-  Decoder getDecoder(InputStream is);
-  Encoder getEncoder(OutputStream os);
+  Decoder getClientDecoder(InputStream is);
+  Encoder getClientEncoder(OutputStream os);
+  Decoder getServerDecoder(InputStream is);
+  Encoder getServerEncoder(OutputStream os);

This also seems 'off':

-    public KeyValueEncoder(final OutputStream out) {
+    private final boolean isClientContext;

Fix the below text:

+ * will be <code>null</code>. The CallRunner class before it a call and then on

It is unorthodox in the hbase codebase having data members midway down the class:

+  private User user;
+  private boolean isSuperUser = false;
+  private InetAddress remoteAddress;
+  // indicates we're within a RPC request invocation
+  private boolean inRequest;

On the below:

-  CallRunner(final RpcServerInterface rpcServer, final Call call, UserProvider userProvider) {
+  CallRunner(final RpcServerInterface rpcServer, final Call call, User user, boolean isSuperUser) {

...can we not pass in some object that contains User info and whether or not it is super user rather than pass User and super user separately?  (It should be called superUser, not isSuperUser).

RequestContext no longer takes service (see below)?

-        RequestContext.set(userProvider.create(call.connection.user), RpcServer.getRemoteIp(),
-          call.connection.service);
+        InetAddress remoteAddress = RpcServer.getRemoteIp();
+        RequestContext.set(user, isSuperUser, remoteAddress);

Should this stuff below be in a finally?

+      RequestContext.clear();

Yeah, this seems wrong Anoop:

-          ipcUtil.buildCellBlock(this.connection.codec, this.connection.compressionCodec, cells);
+        ByteBuffer cellBlock = ipcUtil.buildCellBlock(this.connection.codec,
+            this.connection.compressionCodec, cells, false);

The bit where it is passing flag if server or client.  Can this not be encapsulated inside in the codec?

Pardon me Anoop but I don't like the way this is done.  This may the only way to implement it but I'd like to hear argument why so and why we can't encapsulate the encode/decode in the codec implementation rather than leak client/server context beyond codec'ing.

Good stuff Anoop.

> Strip tags from KV while sending back to client on reads
> --------------------------------------------------------
>
>                 Key: HBASE-10322
>                 URL: https://issues.apache.org/jira/browse/HBASE-10322
>             Project: HBase
>          Issue Type: Bug
>    Affects Versions: 0.98.0
>            Reporter: Anoop Sam John
>            Assignee: Anoop Sam John
>            Priority: Blocker
>             Fix For: 0.98.0, 0.99.0
>
>         Attachments: HBASE-10322.patch
>
>
> Right now we have some inconsistency wrt sending back tags on read. We do this in scan when using Java client(Codec based cell block encoding). But during a Get operation or when a pure PB based Scan comes we are not sending back the tags.  So any of the below fix we have to do
> 1. Send back tags in missing cases also. But sending back visibility expression/ cell ACL is not correct.
> 2. Don't send back tags in any case. This will a problem when a tool like ExportTool use the scan to export the table data. We will miss exporting the cell visibility/ACL.
> 3. Send back tags based on some condition. It has to be per scan basis. Simplest way is pass some kind of attribute in Scan which says whether to send back tags or not. But believing some thing what scan specifies might not be correct IMO. Then comes the way of checking the user who is doing the scan. When a HBase super user doing the scan then only send back tags. So when a case comes like Export Tool's the execution should happen from a super user.
> So IMO we should go with #3.
> Patch coming soon.


--
This message was sent by Atlassian JIRA
(v6.1.5#6160)