Details

    • Hadoop Flags:
      Reviewed
    • Tags:
      0.96notable

      Description

      Introduce Protocol Buffer RPC engine in the RPC core. Protocols that are PB aware can be made to go through this RPC engine. The approach, in my current thinking, would be similar to HADOOP-7773.

      1. 5705-1.patch
        49 kB
        Devaraj Das
      2. 5705-2.1.patch
        252 kB
        Ted Yu
      3. 5705-2.2.patch
        251 kB
        Devaraj Das
      4. 5705-2.3.patch
        251 kB
        Devaraj Das

        Activity

        Devaraj Das created issue -
        Devaraj Das made changes -
        Field Original Value New Value
        Component/s ipc [ 12312136 ]
        Component/s master [ 12312138 ]
        Component/s migration [ 12315703 ]
        Component/s regionserver [ 12312139 ]
        Devaraj Das made changes -
        Assignee Devaraj Das [ devaraj ]
        Hide
        Devaraj Das added a comment -

        Early patch that at least builds. Still working on it.

        Show
        Devaraj Das added a comment - Early patch that at least builds. Still working on it.
        Devaraj Das made changes -
        Attachment 5705-1.patch [ 12533726 ]
        Hide
        Devaraj Das added a comment -

        https://reviews.apache.org/r/5714/ is the RB page with the patch..

        Show
        Devaraj Das added a comment - https://reviews.apache.org/r/5714/ is the RB page with the patch..
        Hide
        Ted Yu added a comment -

        HBASE-6039 has removed HMasterInterface.
        Please adjust patch accordingly.

        Show
        Ted Yu added a comment - HBASE-6039 has removed HMasterInterface. Please adjust patch accordingly.
        Hide
        Devaraj Das added a comment -

        Thanks for looking at the patch, Ted. I'll update it soon.

        Show
        Devaraj Das added a comment - Thanks for looking at the patch, Ted. I'll update it soon.
        Hide
        stack added a comment -

        I added some comments up in RB. Seems like pb stuff goes via Writables still? Would be nice if I did not have to read hadoop-7773 patch to figure out what this change is doing. Any chance of a sentence or two on intent? Good stuff DD.

        Show
        stack added a comment - I added some comments up in RB. Seems like pb stuff goes via Writables still? Would be nice if I did not have to read hadoop-7773 patch to figure out what this change is doing. Any chance of a sentence or two on intent? Good stuff DD.
        Hide
        Ted Yu added a comment -

        Patch from Devaraj.
        I verified that it compiles against latest trunk.

        Show
        Ted Yu added a comment - Patch from Devaraj. I verified that it compiles against latest trunk.
        Ted Yu made changes -
        Attachment 5705-2.1.patch [ 12535753 ]
        Ted Yu made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Fix Version/s 0.96.0 [ 12320040 ]
        Hide
        Devaraj Das added a comment -

        I updated RB with the new patch. Yes, the PB stuff goes via writables still. Some of the stuff can be put within a if/else logic for PB vs. Writable but when I did it, it seemed to make the code complex. So am thinking of doing the work of making RPC use as much of PB as possible as a follow up (and at that time, remove the WritableRpcEngine/Invocation classes), when all the application protocols are converted to PB. Open to feedback on this aspect.
        Once the above is done, the use of PB RPC Engine should make things more efficient in terms of avoiding copies in the rpc layer (for example in the RPC layer, if we needed to write a pair of

        {message-length, message}

        , in the current RPC, we would need to serialize the Writable object into a buffer, and then get the length of the buffer. In the PB world, every message has a getSerializedSize method generated..)

        Show
        Devaraj Das added a comment - I updated RB with the new patch. Yes, the PB stuff goes via writables still. Some of the stuff can be put within a if/else logic for PB vs. Writable but when I did it, it seemed to make the code complex. So am thinking of doing the work of making RPC use as much of PB as possible as a follow up (and at that time, remove the WritableRpcEngine/Invocation classes), when all the application protocols are converted to PB. Open to feedback on this aspect. Once the above is done, the use of PB RPC Engine should make things more efficient in terms of avoiding copies in the rpc layer (for example in the RPC layer, if we needed to write a pair of {message-length, message} , in the current RPC, we would need to serialize the Writable object into a buffer, and then get the length of the buffer. In the PB world, every message has a getSerializedSize method generated..)
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12535753/5705-2.1.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 17 new or modified tests.

        +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        -1 javac. The applied patch generated 5 javac compiler warnings (more than the trunk's current 4 warnings).

        -1 findbugs. The patch appears to introduce 9 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.coprocessor.TestRegionServerCoprocessorExceptionWithAbort

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/2356//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2356//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2356//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2356//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12535753/5705-2.1.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 17 new or modified tests. +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The applied patch generated 5 javac compiler warnings (more than the trunk's current 4 warnings). -1 findbugs. The patch appears to introduce 9 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.coprocessor.TestRegionServerCoprocessorExceptionWithAbort Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/2356//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2356//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2356//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2356//console This message is automatically generated.
        Hide
        stack added a comment -

        I'd vote for removing Writables engine once all pb converted.

        Show
        stack added a comment - I'd vote for removing Writables engine once all pb converted.
        Hide
        stack added a comment -

        RB is not working for me at mo so reviews here:

        I asked about comment in ClientCache in previous reviews but it went unanswered. I'm talking about the old Doug comment on a. vs. b. choice. I didn't understand it then and less so now. Is it a copy/paste error?

        The writing to a buffer before we write on the line goes away if we move to a pure pb engine?

        Is the size used here right for case where we have an exception? (Or its just a hint?)

        ByteBufferOutputStream buf = new ByteBufferOutputStream(size);

        And RPCRequestWritable and RPCResponseWritable will go away too when we do pb? (You've answered yes to this multiple times I believe).

        Why the need for a setRPC w/ no params?

        This stuff passed to Server, whats it about? Is this some Hadoop thing about being able to do multiple serializations?

        Class<? extends Writable> paramClass,

        When we do logResponse, we no longer take a Call but we pass in its name and methodnames and params.... is that a regression? Would it be better to take the Invocation?

        The below doesn't look too bad:

        <"hrpc"-bytearray><'5'-byte><length-of-serialized-ConnectionHeader-obj[int32]><ConnectionHeader-object serialized>

        In header can we say if its pbs that follow? Or will we have to always calc message size before sending?

        Why we need this: clientProtocolVersion in request? Is this Writables thing? It goes away when we go pb?

        My general thought on this is we commit. This patch has same shape as our current rpc'ing; same classes, etc. just moved over some to support pb. We need to go pure pb and get rid of Writables in this rpc call path. I see this as a stepping stone so we should get it in so we can undo it later w/ pb engine.

        Show
        stack added a comment - RB is not working for me at mo so reviews here: I asked about comment in ClientCache in previous reviews but it went unanswered. I'm talking about the old Doug comment on a. vs. b. choice. I didn't understand it then and less so now. Is it a copy/paste error? The writing to a buffer before we write on the line goes away if we move to a pure pb engine? Is the size used here right for case where we have an exception? (Or its just a hint?) ByteBufferOutputStream buf = new ByteBufferOutputStream(size); And RPCRequestWritable and RPCResponseWritable will go away too when we do pb? (You've answered yes to this multiple times I believe). Why the need for a setRPC w/ no params? This stuff passed to Server, whats it about? Is this some Hadoop thing about being able to do multiple serializations? Class<? extends Writable> paramClass, When we do logResponse, we no longer take a Call but we pass in its name and methodnames and params.... is that a regression? Would it be better to take the Invocation? The below doesn't look too bad: <"hrpc"-bytearray><'5'-byte><length-of-serialized-ConnectionHeader-obj [int32] ><ConnectionHeader-object serialized> In header can we say if its pbs that follow? Or will we have to always calc message size before sending? Why we need this: clientProtocolVersion in request? Is this Writables thing? It goes away when we go pb? My general thought on this is we commit. This patch has same shape as our current rpc'ing; same classes, etc. just moved over some to support pb. We need to go pure pb and get rid of Writables in this rpc call path. I see this as a stepping stone so we should get it in so we can undo it later w/ pb engine.
        Hide
        Devaraj Das added a comment -

        Sorry, seems like I forgot to "publish" my responses to the comments on RB on some occasions. I "published" all of them now. Apologize for the out-of-order responses. I'll respond to your comments shortly, Stack (thanks for looking).

        Show
        Devaraj Das added a comment - Sorry, seems like I forgot to "publish" my responses to the comments on RB on some occasions. I "published" all of them now. Apologize for the out-of-order responses. I'll respond to your comments shortly, Stack (thanks for looking).
        Hide
        stack added a comment -

        np I responded up on rb. See my other comments above (ignore stuff that is duplicated and already answered). Any useful stuff in above feedback? If so, address and lets get this in.

        Show
        stack added a comment - np I responded up on rb. See my other comments above (ignore stuff that is duplicated and already answered). Any useful stuff in above feedback? If so, address and lets get this in.
        Hide
        Devaraj Das added a comment -

        I asked about comment in ClientCache in previous reviews but it went unanswered. I'm talking about the old Doug comment on a. vs. b. choice. I didn't understand it then and less so now. Is it a copy/paste error?

        Answered on RB but a little bit more here - I think it makes sense to remove this confusing comment (since now the ClientCache class has some javadoc). I'll remove it in the next patch.

        The writing to a buffer before we write on the line goes away if we move to a pure pb engine?

        Right.

        Is the size used here right for case where we have an exception? (Or its just a hint?)

        Hint.

        And RPCRequestWritable and RPCResponseWritable will go away too when we do pb? (You've answered yes to this multiple times I believe).

        Yes (AFAICT).

        Why the need for a setRPC w/ no params?

        Could you please point me to the block of code in the patch where this is.. If you are referring to the setRPC with two params that I added here, that I reverted back in my current update.. (and added a comment up on HBASE-6282).

        This stuff passed to Server, whats it about? Is this some Hadoop thing about being able to do multiple serializations?

        This is to handle WritableRpcEngine and ProtobufRpcEngine equally well. In the case of WritableRpcEngine, the RPC request is serialized using Invocation objects and in the case of ProtobufRpcEngine, they are done using RpcRequestWritable (a thin wrapper over PB objects).

        When we do logResponse, we no longer take a Call but we pass in its name and methodnames and params.... is that a regression? Would it be better to take the Invocation?

        This is handled in the way that the method now takes methodname and params explicitly (as opposed to how it is in the current trunk where an Invocation object is passed and the method body extracts the params and methodname). This was needed to support ProtobufRpc. Also, HBASE-6282 is applicable here.

        In header can we say if its pbs that follow? Or will we have to always calc message size before sending?

        The RPC requires that any RPC message is preceded by the length of the same. In that sense, we need the message size.

        Why we need this: clientProtocolVersion in request? Is this Writables thing? It goes away when we go pb?

        The clientProtocolVersion is currently unused on the server side (and it's optional in the .proto definition), but it could potentially be used in the server to decide how to best service the client's RPC (if there were multiple implementations for a method, and the server could pick an implementation based on client version). If this use case seems like we probably won't need to support, we can drop the clientProtocolVersion (btw it being optional in the .proto definition makes it amenable to easy removal even later). By keeping it, it doesn't hurt either..

        Show
        Devaraj Das added a comment - I asked about comment in ClientCache in previous reviews but it went unanswered. I'm talking about the old Doug comment on a. vs. b. choice. I didn't understand it then and less so now. Is it a copy/paste error? Answered on RB but a little bit more here - I think it makes sense to remove this confusing comment (since now the ClientCache class has some javadoc). I'll remove it in the next patch. The writing to a buffer before we write on the line goes away if we move to a pure pb engine? Right. Is the size used here right for case where we have an exception? (Or its just a hint?) Hint. And RPCRequestWritable and RPCResponseWritable will go away too when we do pb? (You've answered yes to this multiple times I believe). Yes (AFAICT). Why the need for a setRPC w/ no params? Could you please point me to the block of code in the patch where this is.. If you are referring to the setRPC with two params that I added here, that I reverted back in my current update.. (and added a comment up on HBASE-6282 ). This stuff passed to Server, whats it about? Is this some Hadoop thing about being able to do multiple serializations? This is to handle WritableRpcEngine and ProtobufRpcEngine equally well. In the case of WritableRpcEngine, the RPC request is serialized using Invocation objects and in the case of ProtobufRpcEngine, they are done using RpcRequestWritable (a thin wrapper over PB objects). When we do logResponse, we no longer take a Call but we pass in its name and methodnames and params.... is that a regression? Would it be better to take the Invocation? This is handled in the way that the method now takes methodname and params explicitly (as opposed to how it is in the current trunk where an Invocation object is passed and the method body extracts the params and methodname). This was needed to support ProtobufRpc. Also, HBASE-6282 is applicable here. In header can we say if its pbs that follow? Or will we have to always calc message size before sending? The RPC requires that any RPC message is preceded by the length of the same. In that sense, we need the message size. Why we need this: clientProtocolVersion in request? Is this Writables thing? It goes away when we go pb? The clientProtocolVersion is currently unused on the server side (and it's optional in the .proto definition), but it could potentially be used in the server to decide how to best service the client's RPC (if there were multiple implementations for a method, and the server could pick an implementation based on client version). If this use case seems like we probably won't need to support, we can drop the clientProtocolVersion (btw it being optional in the .proto definition makes it amenable to easy removal even later). By keeping it, it doesn't hurt either..
        Devaraj Das made changes -
        Attachment 5705-2.2.patch [ 12537940 ]
        Hide
        Devaraj Das added a comment -

        My general thought on this is we commit. This patch has same shape as our current rpc'ing; same classes, etc. just moved over some to support pb. We need to go pure pb and get rid of Writables in this rpc call path. I see this as a stepping stone so we should get it in so we can undo it later w/ pb engine.

        +1

        Show
        Devaraj Das added a comment - My general thought on this is we commit. This patch has same shape as our current rpc'ing; same classes, etc. just moved over some to support pb. We need to go pure pb and get rid of Writables in this rpc call path. I see this as a stepping stone so we should get it in so we can undo it later w/ pb engine. +1
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12537940/5705-2.2.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 17 new or modified tests.

        +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        -1 javac. The applied patch generated 5 javac compiler warnings (more than the trunk's current 4 warnings).

        -1 findbugs. The patch appears to introduce 15 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/2438//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2438//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2438//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2438//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2438//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2438//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2438//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12537940/5705-2.2.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 17 new or modified tests. +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The applied patch generated 5 javac compiler warnings (more than the trunk's current 4 warnings). -1 findbugs. The patch appears to introduce 15 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/2438//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2438//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2438//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2438//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2438//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2438//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2438//console This message is automatically generated.
        Hide
        stack added a comment -

        All of above seems reasonable. Here is some response.

        ...Could you please point me to the block of code in the patch where this is.

        I do not see it now.

        ...and in the case of ProtobufRpcEngine, they are done using RpcRequestWritable (a thin wrapper over PB objects).

        The Writable wrapper goes away when we move to pb engine?

        ...In that sense, we need the message size.

        Its header length only right? Thats ok I'd say. But for the value, when its a 50M cell, we won't have to read it into a buffer to find its size before sending will we when we are pb? (pb does this internally probably but we do we have to do it again outside of pb to put it in rpc header?)

        Show
        stack added a comment - All of above seems reasonable. Here is some response. ...Could you please point me to the block of code in the patch where this is. I do not see it now. ...and in the case of ProtobufRpcEngine, they are done using RpcRequestWritable (a thin wrapper over PB objects). The Writable wrapper goes away when we move to pb engine? ...In that sense, we need the message size. Its header length only right? Thats ok I'd say. But for the value, when its a 50M cell, we won't have to read it into a buffer to find its size before sending will we when we are pb? (pb does this internally probably but we do we have to do it again outside of pb to put it in rpc header?)
        Hide
        Devaraj Das added a comment -

        The Writable wrapper goes away when we move to pb engine?

        Yes (AFAICT).

        Its header length only right? Thats ok I'd say. But for the value, when its a 50M cell, we won't have to read it into a buffer to find its size before sending will we when we are pb? (pb does this internally probably but we do we have to do it again outside of pb to put it in rpc header?)

        No we don't have to write to a buffer in the case of PB objects. There is a getSerializedSize method on PB objects (and assuming that the 50M cell is encapsulated in the PB object as a byte-array, PB would simply call byte-array.length...).

        Show
        Devaraj Das added a comment - The Writable wrapper goes away when we move to pb engine? Yes (AFAICT). Its header length only right? Thats ok I'd say. But for the value, when its a 50M cell, we won't have to read it into a buffer to find its size before sending will we when we are pb? (pb does this internally probably but we do we have to do it again outside of pb to put it in rpc header?) No we don't have to write to a buffer in the case of PB objects. There is a getSerializedSize method on PB objects (and assuming that the 50M cell is encapsulated in the PB object as a byte-array, PB would simply call byte-array.length...).
        Hide
        stack added a comment -

        I'm going to commit this in next day unless objection.

        Show
        stack added a comment - I'm going to commit this in next day unless objection.
        Hide
        Devaraj Das added a comment -

        Thanks for the reviews, Stack & Ted.

        Show
        Devaraj Das added a comment - Thanks for the reviews, Stack & Ted.
        Hide
        Devaraj Das added a comment -

        Noticed that the patch went stale. Updated. (Can this please be committed.)

        Show
        Devaraj Das added a comment - Noticed that the patch went stale. Updated. (Can this please be committed.)
        Devaraj Das made changes -
        Attachment 5705-2.3.patch [ 12538291 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12538291/5705-2.3.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 17 new or modified tests.

        +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        -1 javac. The applied patch generated 5 javac compiler warnings (more than the trunk's current 4 warnings).

        -1 findbugs. The patch appears to introduce 6 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.replication.TestMasterReplication

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/2452//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2452//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2452//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2452//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2452//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2452//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2452//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12538291/5705-2.3.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 17 new or modified tests. +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The applied patch generated 5 javac compiler warnings (more than the trunk's current 4 warnings). -1 findbugs. The patch appears to introduce 6 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.replication.TestMasterReplication Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/2452//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2452//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2452//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2452//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2452//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/2452//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/2452//console This message is automatically generated.
        Hide
        stack added a comment -

        Committed to trunk. Thanks Deveraj.

        Show
        stack added a comment - Committed to trunk. Thanks Deveraj.
        stack made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Hadoop Flags Reviewed [ 10343 ]
        Resolution Fixed [ 1 ]
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #3183 (See https://builds.apache.org/job/HBase-TRUNK/3183/)
        HBASE-5705 Introduce Protocol Buffer RPC engine (Revision 1367009)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ClientCache.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ProtobufRpcEngine.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        • /hbase/trunk/hbase-server/src/main/protobuf/RPC.proto
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestProtoBufRpc.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/protobuf
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/protobuf/generated
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/protobuf/generated/TestProtos.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/protobuf/generated/TestRpcServiceProtos.java
        • /hbase/trunk/hbase-server/src/test/protobuf
        • /hbase/trunk/hbase-server/src/test/protobuf/test.proto
        • /hbase/trunk/hbase-server/src/test/protobuf/test_rpc_service.proto
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #3183 (See https://builds.apache.org/job/HBase-TRUNK/3183/ ) HBASE-5705 Introduce Protocol Buffer RPC engine (Revision 1367009) Result = FAILURE stack : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ClientCache.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ProtobufRpcEngine.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/hbase-server/src/main/protobuf/RPC.proto /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestProtoBufRpc.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/protobuf /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/protobuf/generated /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/protobuf/generated/TestProtos.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/protobuf/generated/TestRpcServiceProtos.java /hbase/trunk/hbase-server/src/test/protobuf /hbase/trunk/hbase-server/src/test/protobuf/test.proto /hbase/trunk/hbase-server/src/test/protobuf/test_rpc_service.proto
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #115 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/115/)
        HBASE-5705 Introduce Protocol Buffer RPC engine (Revision 1367009)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ClientCache.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ProtobufRpcEngine.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java
        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
        • /hbase/trunk/hbase-server/src/main/protobuf/RPC.proto
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestProtoBufRpc.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/protobuf
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/protobuf/generated
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/protobuf/generated/TestProtos.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/protobuf/generated/TestRpcServiceProtos.java
        • /hbase/trunk/hbase-server/src/test/protobuf
        • /hbase/trunk/hbase-server/src/test/protobuf/test.proto
        • /hbase/trunk/hbase-server/src/test/protobuf/test_rpc_service.proto
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #115 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/115/ ) HBASE-5705 Introduce Protocol Buffer RPC engine (Revision 1367009) Result = FAILURE stack : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ClientCache.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/HBaseClient.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/HBaseRPC.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/HBaseServer.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/Invocation.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ProtobufRpcEngine.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/WritableRpcEngine.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/monitoring/MonitoredRPCHandlerImpl.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java /hbase/trunk/hbase-server/src/main/protobuf/RPC.proto /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestProtoBufRpc.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/protobuf /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/protobuf/generated /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/protobuf/generated/TestProtos.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/protobuf/generated/TestRpcServiceProtos.java /hbase/trunk/hbase-server/src/test/protobuf /hbase/trunk/hbase-server/src/test/protobuf/test.proto /hbase/trunk/hbase-server/src/test/protobuf/test_rpc_service.proto
        Hide
        stack added a comment -

        It looks like this patch made it so that when we set up the proxy, we no longer ask the server what its version of the protocol is. Instead we return the client's version of the protocol. See below.

        + static long getProtocolVersion(Class<? extends VersionedProtocol> protocol)
        + throws NoSuchFieldException, IllegalAccessException

        { + Field versionField = protocol.getField("VERSION"); + versionField.setAccessible(true); + return versionField.getLong(protocol); + }

        Was expediency the reason the querying of server version was undone? Something to be addressed later when VersionedProtocol was fixed up?

        Thanks.

        Show
        stack added a comment - It looks like this patch made it so that when we set up the proxy, we no longer ask the server what its version of the protocol is. Instead we return the client's version of the protocol. See below. + static long getProtocolVersion(Class<? extends VersionedProtocol> protocol) + throws NoSuchFieldException, IllegalAccessException { + Field versionField = protocol.getField("VERSION"); + versionField.setAccessible(true); + return versionField.getLong(protocol); + } Was expediency the reason the querying of server version was undone? Something to be addressed later when VersionedProtocol was fixed up? Thanks.
        Hide
        Devaraj Das added a comment -

        stack, yes. The idea was to have the server decide how to best handle the rpc method call based on the client's protocol version. Though the server currently ignores the version, this is something that can be used to implement versioning. Please have a look at ProtobufRpcServerEngine.java around the area with the comment "TODO: use the clientVersion..". Other than that, HBASE-6521 should address versioning in a more complete fashion (and I guess we should unify all the versioning stories there).

        Show
        Devaraj Das added a comment - stack , yes. The idea was to have the server decide how to best handle the rpc method call based on the client's protocol version. Though the server currently ignores the version, this is something that can be used to implement versioning. Please have a look at ProtobufRpcServerEngine.java around the area with the comment "TODO: use the clientVersion..". Other than that, HBASE-6521 should address versioning in a more complete fashion (and I guess we should unify all the versioning stories there).
        stack made changes -
        Fix Version/s 0.95.0 [ 12324094 ]
        Fix Version/s 0.96.0 [ 12320040 ]
        stack made changes -
        Fix Version/s 0.98.0 [ 12323143 ]
        stack made changes -
        Fix Version/s 0.98.0 [ 12323143 ]
        Hide
        stack added a comment -

        Marking closed.

        Show
        stack added a comment - Marking closed.
        stack made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        stack made changes -
        Tags 0.96notable
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Patch Available Patch Available
        97d 3h 51m 1 Ted Yu 09/Jul/12 23:27
        Patch Available Patch Available Resolved Resolved
        20d 8h 40m 1 stack 30/Jul/12 08:08
        Resolved Resolved Closed Closed
        420d 11h 22m 1 stack 23/Sep/13 19:30

          People

          • Assignee:
            Devaraj Das
            Reporter:
            Devaraj Das
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development