Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.98.0, 0.95.0
    • Component/s: IPC/RPC
    • Labels:
      None
    • Tags:
      0.96notable

      Description

      RPC format is changing for 0.96 to accomodate our protobufing all around. Here is a first cut. Please shred: https://docs.google.com/document/d/1-1RJMLXzYldmHgKP7M7ynK6euRpucD03fZ603DlZfGI/edit

      1. 7533_proto_sketch.txt
        16 kB
        stack
      2. 7533.txt
        0.9 kB
        stack
      3. 7533v4.txt
        322 kB
        stack
      4. addendum.txt
        0.2 kB
        stack
      5. rpc_spec.txt
        244 kB
        stack
      6. rpc_spec3.txt
        278 kB
        Devaraj Das
      7. rpc_spec3.txt
        278 kB
        stack

        Activity

        Hide
        Elliott Clark added a comment - - edited

        So this exercise got me thinking. Right now RpcRequstHeader and RpcResponseHeader are used so that not all data must be decoded to get some meta data. Since we're moving towards EncodedDataBlocks coming after the Protobuf responses, there's not a requirement for a header.

        That means we could have one RpcRequest and one RpcResponse. That would greatly simplify the rpc specification.

        message CoprocessorServiceRequest {
          required RegionSpecifier region = 1;
          required CoprocessorServiceCall call = 2;
        }
        message MutateRequest {
          required RegionSpecifier region = 1;
          required Mutate mutate = 2;
          optional Condition condition = 3;
        }
        message GetRequest {
          required RegionSpecifier region = 1;
          required Get get = 2;
        
          // If the row to get doesn't exist, return the
          // closest row before.
          optional bool closestRowBefore = 3;
        
          // The result isn't asked for, just check for
          // the existence. If specified, closestRowBefore
          // will be ignored
          optional bool existenceOnly = 4;
        }
        
        message ScanRequest {
          optional RegionSpecifier region = 1;
          optional Scan scan = 2;
          optional uint64 scannerId = 3;
          optional uint32 numberOfRows = 4;
          optional bool closeScanner = 5;
          optional uint64 nextCallSeq = 6;
        }
        message RpcException {
          /** Class name of the exception thrown from the server */
          required string exceptionName = 1;
        
          /** Exception stack trace from the server side */
          optional string stackTrace = 2;
        }
        message UnionRequestType {
        	required Enum requestType {GET = 1; MUTATE = 2; SCAN = 3; BULKLOAD = 4; COPROC = 5;} = 1;
        	optional GetRequest getRequest = 2
        	optional MutateRequest mutateRequest = 3;
        	optional ScanRequest scanRequest = 4;
        	optional BulkLoadRequest bulkLoadRequest = 5;
        	optional CoprocessorServiceRequest coprocessorServiceRequest = 6;
        	optional RpcException exception = 7;
        }
        message TraceInfo {
         //Whatever is here
        }
         
        message EncodedDateBlockMeta {
        	required unit64 = size =1;
        	optional string type = 2 [default = "KeyValue"]
        	optional encoding encoding = 3;
        }
        
        message RpcRequest {
        	required long callId = 1;
        	required ServiceDescriptor serviceId = 2; // Equivilant to current protocol.
        	required MethodDescriptor method = 3;
        	optional EncodedDataBlockMeta encodedDataMeta = 4; 
        	repeated UnionRequestType requests = 5;
        	optinal TraceInfo traceInfo = 6;
        	optional unit8 priority = 7 [default = 0];
        
        }
        message UnionResponseType {
        	required Enum responseType {GET = 1; MUTATE = 2; SCAN = 3; BULKLOAD = 4; COPROC = 5;} = 1;
        	optional GetResponse getResponse  = 2
        	optional MutateResponse  mutateResponse  = 3;
        	optional ScanResponse  scanResponse  = 4;
        	optional BulkLoadResponse  bulkLoadResponse  = 5;
        	optional CoprocessorServiceResponse  coprocessorResponse  = 6
        }
        
        message RpcResponse {
        	required unit64 callId = 1;
        	required ServiceDescriptor serviceId = 2; // Equivilant to current protocol.
        	required MethodDescriptor method = 3;
        	repeated UnionResponseType requests = 5;
        
        }
        
        
        Show
        Elliott Clark added a comment - - edited So this exercise got me thinking. Right now RpcRequstHeader and RpcResponseHeader are used so that not all data must be decoded to get some meta data. Since we're moving towards EncodedDataBlocks coming after the Protobuf responses, there's not a requirement for a header. That means we could have one RpcRequest and one RpcResponse. That would greatly simplify the rpc specification. message CoprocessorServiceRequest { required RegionSpecifier region = 1; required CoprocessorServiceCall call = 2; } message MutateRequest { required RegionSpecifier region = 1; required Mutate mutate = 2; optional Condition condition = 3; } message GetRequest { required RegionSpecifier region = 1; required Get get = 2; // If the row to get doesn't exist, return the // closest row before. optional bool closestRowBefore = 3; // The result isn't asked for , just check for // the existence. If specified, closestRowBefore // will be ignored optional bool existenceOnly = 4; } message ScanRequest { optional RegionSpecifier region = 1; optional Scan scan = 2; optional uint64 scannerId = 3; optional uint32 numberOfRows = 4; optional bool closeScanner = 5; optional uint64 nextCallSeq = 6; } message RpcException { /** Class name of the exception thrown from the server */ required string exceptionName = 1; /** Exception stack trace from the server side */ optional string stackTrace = 2; } message UnionRequestType { required Enum requestType {GET = 1; MUTATE = 2; SCAN = 3; BULKLOAD = 4; COPROC = 5;} = 1; optional GetRequest getRequest = 2 optional MutateRequest mutateRequest = 3; optional ScanRequest scanRequest = 4; optional BulkLoadRequest bulkLoadRequest = 5; optional CoprocessorServiceRequest coprocessorServiceRequest = 6; optional RpcException exception = 7; } message TraceInfo { //Whatever is here } message EncodedDateBlockMeta { required unit64 = size =1; optional string type = 2 [ default = "KeyValue" ] optional encoding encoding = 3; } message RpcRequest { required long callId = 1; required ServiceDescriptor serviceId = 2; // Equivilant to current protocol. required MethodDescriptor method = 3; optional EncodedDataBlockMeta encodedDataMeta = 4; repeated UnionRequestType requests = 5; optinal TraceInfo traceInfo = 6; optional unit8 priority = 7 [ default = 0]; } message UnionResponseType { required Enum responseType {GET = 1; MUTATE = 2; SCAN = 3; BULKLOAD = 4; COPROC = 5;} = 1; optional GetResponse getResponse = 2 optional MutateResponse mutateResponse = 3; optional ScanResponse scanResponse = 4; optional BulkLoadResponse bulkLoadResponse = 5; optional CoprocessorServiceResponse coprocessorResponse = 6 } message RpcResponse { required unit64 callId = 1; required ServiceDescriptor serviceId = 2; // Equivilant to current protocol. required MethodDescriptor method = 3; repeated UnionResponseType requests = 5; }
        Hide
        stack added a comment -

        Thats better. Much better. I'll update the doc. (You are going to comment on the union... )

        Show
        stack added a comment - Thats better. Much better. I'll update the doc. (You are going to comment on the union... )
        Hide
        stack added a comment -

        Chatting with Elliott Clark, a "downside" to the above scheme is the need in protobuf to list every ipc method in the UnionResponseType enum and also in the UnionRequestType. It seems a bit much given we can extrapolate param type and return given the method name (whether we are doing reflection against 'protocol' Interfaces or lookups in pb Service). Elliott suggested we could have opaque bytes for the request and response Message. This would mean unmarshal the RpcResponse, then unmarshal the contained bytes to find the Response Message. This would be a bit of a pain. Where we left it was prototyping out both; that would be probably more informative than prognosticating in front of a white board. I'll have a go at it.

        Hey Elliott Clark, is there a response type missing from your enum example list above? The error type?

        Show
        stack added a comment - Chatting with Elliott Clark , a "downside" to the above scheme is the need in protobuf to list every ipc method in the UnionResponseType enum and also in the UnionRequestType. It seems a bit much given we can extrapolate param type and return given the method name (whether we are doing reflection against 'protocol' Interfaces or lookups in pb Service). Elliott suggested we could have opaque bytes for the request and response Message. This would mean unmarshal the RpcResponse, then unmarshal the contained bytes to find the Response Message. This would be a bit of a pain. Where we left it was prototyping out both; that would be probably more informative than prognosticating in front of a white board. I'll have a go at it. Hey Elliott Clark , is there a response type missing from your enum example list above? The error type?
        Hide
        Elliott Clark added a comment - - edited

        My thought was that the union type would have an optional exception type in addition to the response types; that would allow us to more directly tie exceptions from multis to the action that caused them. Though that might not be great. What do you think ?

        Show
        Elliott Clark added a comment - - edited My thought was that the union type would have an optional exception type in addition to the response types; that would allow us to more directly tie exceptions from multis to the action that caused them. Though that might not be great. What do you think ?
        Hide
        stack added a comment -

        Elliott Clark Well, we need to be able to pick through a multi response and correlate exception and request that caused it so yeah, an exceptionResponse (with an 'exception' addition to the enum...). Or were you thinking something different?

        Show
        stack added a comment - Elliott Clark Well, we need to be able to pick through a multi response and correlate exception and request that caused it so yeah, an exceptionResponse (with an 'exception' addition to the enum...). Or were you thinking something different?
        Hide
        Elliott Clark added a comment -

        The Union message types are structured after https://developers.google.com/protocol-buffers/docs/techniques#union.

        We should move the enum out of the Request/Response messages. That will clean some things up a little.

        As far as the exception being in the enum, my thinking was as follows:

        • For multi we could say what type the response should be.
        • Then if there's an exception put that in the exceptions field.
        • If there was a partial result it could still go in the result field.

        This would allow us to give partial results (a new feature for the 0.98ish time frame). It would also allow us to just give the features that we have currently.

        Show
        Elliott Clark added a comment - The Union message types are structured after https://developers.google.com/protocol-buffers/docs/techniques#union . We should move the enum out of the Request/Response messages. That will clean some things up a little. As far as the exception being in the enum, my thinking was as follows: For multi we could say what type the response should be. Then if there's an exception put that in the exceptions field. If there was a partial result it could still go in the result field. This would allow us to give partial results (a new feature for the 0.98ish time frame). It would also allow us to just give the features that we have currently.
        Hide
        Devaraj Das added a comment -

        Went over the doc briefly. We should have a section on how to handle priority RPCs. Thinking about it, looking at the method name on the server side and then parsing the request object (for example, if the request object has reference to META it should have higher priority) would work to segregate the requests. In the request I see "Get, Put, Scan, etc". Are these the bytestring representation of the PB messages?

        Show
        Devaraj Das added a comment - Went over the doc briefly. We should have a section on how to handle priority RPCs. Thinking about it, looking at the method name on the server side and then parsing the request object (for example, if the request object has reference to META it should have higher priority) would work to segregate the requests. In the request I see "Get, Put, Scan, etc". Are these the bytestring representation of the PB messages?
        Hide
        Elliott Clark added a comment -

        The request should have a priority imo. That will make transitioning to a more granular priority queue much easier. It will also be simpler.

        Show
        Elliott Clark added a comment - The request should have a priority imo. That will make transitioning to a more granular priority queue much easier. It will also be simpler.
        Hide
        Devaraj Das added a comment -

        The request should have a priority imo.

        The thing that we need to then care about is no one should be misusing the priority (maliciously or otherwise). That will be a hole. I think it is the job of the server to decide the priority and not the client's.. In the current RPC implementation the server parses the request object for known request types to figure out whether the request should be treated with higher priority or not. There could be a bunch of other factors that determine the priority (like which IP address the request came from etc.) but request object potentially has certain attributes that play a role here as well..

        Show
        Devaraj Das added a comment - The request should have a priority imo. The thing that we need to then care about is no one should be misusing the priority (maliciously or otherwise). That will be a hole. I think it is the job of the server to decide the priority and not the client's.. In the current RPC implementation the server parses the request object for known request types to figure out whether the request should be treated with higher priority or not. There could be a bunch of other factors that determine the priority (like which IP address the request came from etc.) but request object potentially has certain attributes that play a role here as well..
        Hide
        Elliott Clark added a comment -

        At this point a rogue client can pretty easily bring down a cluster, so I don't think that adding the complexity of a rules based system on the server will really buy us any resiliency. We could be sanity check the priority on the server side.

        There could be a bunch of other factors that determine the priority

        I don't think that we can anticipate all of the factors that users will want to include. So it's easier to let users add their logic on the client side rather than the server side.

        Additionally having the ability to expose priority to the user would be really useful feature that we would get for almost no extra cost. In my eyes that would be a start towards allowing users to have mapred and realtime use cases on the same hardware.

        Show
        Elliott Clark added a comment - At this point a rogue client can pretty easily bring down a cluster, so I don't think that adding the complexity of a rules based system on the server will really buy us any resiliency. We could be sanity check the priority on the server side. There could be a bunch of other factors that determine the priority I don't think that we can anticipate all of the factors that users will want to include. So it's easier to let users add their logic on the client side rather than the server side. Additionally having the ability to expose priority to the user would be really useful feature that we would get for almost no extra cost. In my eyes that would be a start towards allowing users to have mapred and realtime use cases on the same hardware.
        Hide
        Devaraj Das added a comment -

        At this point a rogue client can pretty easily bring down a cluster, so...
        We could be sanity check the priority on the server side.
        I don't think that we can anticipate all of the factors that users will want to include. So it's easier to let users add their logic on the client side rather than the server side.

        Fair points.. For now, let's make the priority an optional field in the RPC and let the client send it. That would give us enough wiggle room to switch to an alternate implementation (and at that point maybe make it configurable on whether we would honor client's notion of priority..).

        I don't think that we can anticipate all of the factors that users will want to include

        Of course.. I meant to just give examples of other things that the server might want to consider before assigning priority to a request...

        Show
        Devaraj Das added a comment - At this point a rogue client can pretty easily bring down a cluster, so... We could be sanity check the priority on the server side. I don't think that we can anticipate all of the factors that users will want to include. So it's easier to let users add their logic on the client side rather than the server side. Fair points.. For now, let's make the priority an optional field in the RPC and let the client send it. That would give us enough wiggle room to switch to an alternate implementation (and at that point maybe make it configurable on whether we would honor client's notion of priority..). I don't think that we can anticipate all of the factors that users will want to include Of course.. I meant to just give examples of other things that the server might want to consider before assigning priority to a request...
        Hide
        Elliott Clark added a comment -

        For now, let's make the priority an optional field in the RPC and let the client send it. That would give us enough wiggle room to switch to an alternate implementation

        Seems like a good way to go forward. I think most of the fields are going to be optional going forward.

        Show
        Elliott Clark added a comment - For now, let's make the priority an optional field in the RPC and let the client send it. That would give us enough wiggle room to switch to an alternate implementation Seems like a good way to go forward. I think most of the fields are going to be optional going forward.
        Hide
        stack added a comment -

        Here is my completing Elliott's sketch. I'm not sure I like it. In particular, I do not like that we have these fat RpcRequest and RpcResponse that have all types listed though they may not apply.

        Pity you can't inherit a base type in protobuf...

        On other hand, not all Services have a callid or want to do tracing...

        Show
        stack added a comment - Here is my completing Elliott's sketch. I'm not sure I like it. In particular, I do not like that we have these fat RpcRequest and RpcResponse that have all types listed though they may not apply. Pity you can't inherit a base type in protobuf... On other hand, not all Services have a callid or want to do tracing...
        Hide
        Enis Soztutar added a comment -

        For HBASE-7268, I was going over the changes in RegionMovedException, and realized that, for some class of exceptions (RegionMoved, RegionOffline, etc), we have to carry some data over the wire to the client side which should be PB'ed as well. Right now, we are sending the stack trace over, and from the client side, parsing the stack trace. i guess this is not handled at the attached spec as well.

        Thinking about the problem with Sergey and DD, we realized that there are mainly two classes of exceptions that we can throw. RegionMovedException, and the like are different from KeeperException, etc, in the sense that they are expected return values for some rpc calls, like put, and they carry data.

        In the end, I think we have to be able to define PB messages for some exceptions:

        message RegionMovedException{
          required ServerName serverName = 1;
          optional int64 openSeqNum = 2;
        }
        

        .
        Now given that, I think we can do the enum approach of enumerating all exception messages in RpcException, and have the ipc layer from the client side construct the actual exception class with passing the Message to the exception c.tor. We can have a superclass like, PBException extends Exception containing a Message, and RegionMovedException extends PBException. The ipc layer from the server side, will know about PBException, and will pass the serialized bytes.

        Alternatively, we can try to embed the exception that the client knows about (RegionMovedException) to the actual response messages, as valid messages. The region server code in this case for example, will not throw an exception, but instead, but the exception data in the response, and pass to rpc layer. The actual RPC response will be SUCCESS, not ERROR, but the client can inspect the response data, and see that exception (and maybe construct and throw an actual exception).

        Show
        Enis Soztutar added a comment - For HBASE-7268 , I was going over the changes in RegionMovedException, and realized that, for some class of exceptions (RegionMoved, RegionOffline, etc), we have to carry some data over the wire to the client side which should be PB'ed as well. Right now, we are sending the stack trace over, and from the client side, parsing the stack trace. i guess this is not handled at the attached spec as well. Thinking about the problem with Sergey and DD, we realized that there are mainly two classes of exceptions that we can throw. RegionMovedException, and the like are different from KeeperException, etc, in the sense that they are expected return values for some rpc calls, like put, and they carry data. In the end, I think we have to be able to define PB messages for some exceptions: message RegionMovedException{ required ServerName serverName = 1; optional int64 openSeqNum = 2; } . Now given that, I think we can do the enum approach of enumerating all exception messages in RpcException, and have the ipc layer from the client side construct the actual exception class with passing the Message to the exception c.tor. We can have a superclass like, PBException extends Exception containing a Message, and RegionMovedException extends PBException. The ipc layer from the server side, will know about PBException, and will pass the serialized bytes. Alternatively, we can try to embed the exception that the client knows about (RegionMovedException) to the actual response messages, as valid messages. The region server code in this case for example, will not throw an exception, but instead, but the exception data in the response, and pass to rpc layer. The actual RPC response will be SUCCESS, not ERROR, but the client can inspect the response data, and see that exception (and maybe construct and throw an actual exception).
        Hide
        stack added a comment -

        Thanks for bringing this up now Enis Soztutar

        i guess this is not handled at the attached spec as well.

        Correct.

        Current implementation is like the old where a flag is set in header to say Call failed and then caller reads the Stringified exception and throws the wrapper exception.

        There are a few types of exception currently: 1. A fatal exception kills the connection (bad auth, wrong rpc version), and 2. an exception that just fails the particular call and we keep going w/ outstanding calls. Now we have a third type, one that is to carry pb data.

        Messing w/ Elliott, given our rpc now does a request that takes a single pb Message and returns a single pb Message response, we toyed w/ making it so every Message was a union; either a response or an exception. Undoing the response you'd test if it an exception and act accordingly, otherwise, process the response. This would seem to be how the google fellas would have you do it (according to old blog response, see #9 response here http://steve.vinoski.net/blog/2008/07/13/protocol-buffers-leaky-rpc/). We chose not to edit all 55 response types so they could take an exception instead and if we did, there'd still be the type #1 and #2s above (The type #2s could probably be corralled as legit exception responses).

        How about we try your alternate proposal for now. I think the tendency will be to evolve all rpc in this direction eventually. It would avoid a bunch of code change just now. On other hand, it makes our spec. messier allowing 3 types of exception rather than two (I started an Exceptions section in spec; it needs filling out still).

        What you reckon?

        Show
        stack added a comment - Thanks for bringing this up now Enis Soztutar i guess this is not handled at the attached spec as well. Correct. Current implementation is like the old where a flag is set in header to say Call failed and then caller reads the Stringified exception and throws the wrapper exception. There are a few types of exception currently: 1. A fatal exception kills the connection (bad auth, wrong rpc version), and 2. an exception that just fails the particular call and we keep going w/ outstanding calls. Now we have a third type, one that is to carry pb data. Messing w/ Elliott, given our rpc now does a request that takes a single pb Message and returns a single pb Message response, we toyed w/ making it so every Message was a union; either a response or an exception. Undoing the response you'd test if it an exception and act accordingly, otherwise, process the response. This would seem to be how the google fellas would have you do it (according to old blog response, see #9 response here http://steve.vinoski.net/blog/2008/07/13/protocol-buffers-leaky-rpc/ ). We chose not to edit all 55 response types so they could take an exception instead and if we did, there'd still be the type #1 and #2s above (The type #2s could probably be corralled as legit exception responses). How about we try your alternate proposal for now. I think the tendency will be to evolve all rpc in this direction eventually. It would avoid a bunch of code change just now. On other hand, it makes our spec. messier allowing 3 types of exception rather than two (I started an Exceptions section in spec; it needs filling out still). What you reckon?
        Hide
        stack added a comment -

        So, abandoning the approach represented by the attached patch. Instead going back to:

        <header>
        <request Message>
        <optional encoded data block>

        Going the above route, you have to decode two Messages – we were trying to get it so you only decode one Message with the header info and request Message all in one but to do so, you would have to go the route of the unmaintainable fat rpc proto patch attached. Will update the spec soon.

        Show
        stack added a comment - So, abandoning the approach represented by the attached patch. Instead going back to: <header> <request Message> <optional encoded data block> Going the above route, you have to decode two Messages – we were trying to get it so you only decode one Message with the header info and request Message all in one but to do so, you would have to go the route of the unmaintainable fat rpc proto patch attached. Will update the spec soon.
        Hide
        Elliott Clark added a comment -

        We could still go the route of bytes in a single message. That seems like it would hold with the exceptions problem as well something like

        message StringBasedException {
         required string exceptionClass = 1;
         optional string stack = 2;
        }
        message RegionMovedException {
          required ServerName serverName = 1;
          optional int64 openSeqNum = 2;
        }
        message Response {
          optional bytes responseBody = 1; // builder is infered from method called
          //if the response has extra data put it here.
          optional ECDMeta ecdMetaDate = 2;
        
          optional string exceptionMessageType = 3;
          //use the string to get the builder.
          optional bytes exceptionBody = 4;
        }
        
        Show
        Elliott Clark added a comment - We could still go the route of bytes in a single message. That seems like it would hold with the exceptions problem as well something like message StringBasedException { required string exceptionClass = 1; optional string stack = 2; } message RegionMovedException { required ServerName serverName = 1; optional int64 openSeqNum = 2; } message Response { optional bytes responseBody = 1; // builder is infered from method called // if the response has extra data put it here. optional ECDMeta ecdMetaDate = 2; optional string exceptionMessageType = 3; //use the string to get the builder. optional bytes exceptionBody = 4; }
        Hide
        Devaraj Das added a comment -

        IMHO we should retain the RegionMovedException as an exception (with an embedded Message field that basically is used for serializing the exception fields). Maybe, have such exceptions implement an interface like PBException that has methods that would allow them to be ser/de. Then in the RPC layer it could do instanceof checks and ser/de such exceptions. This would prevent parsing strings while providing PB extensibility to RPC exceptions.
        Do we need to encapsulate the data within a "Response" message? Instead, if we state in the RPC response header that there is an exception (with the exceptionMessageType) following, we could directly serialize the exception. Ditto for the responseBody.. We would avoid copies that way.
        Thoughts?

        Show
        Devaraj Das added a comment - IMHO we should retain the RegionMovedException as an exception (with an embedded Message field that basically is used for serializing the exception fields). Maybe, have such exceptions implement an interface like PBException that has methods that would allow them to be ser/de. Then in the RPC layer it could do instanceof checks and ser/de such exceptions. This would prevent parsing strings while providing PB extensibility to RPC exceptions. Do we need to encapsulate the data within a "Response" message? Instead, if we state in the RPC response header that there is an exception (with the exceptionMessageType) following, we could directly serialize the exception. Ditto for the responseBody.. We would avoid copies that way. Thoughts?
        Hide
        Enis Soztutar added a comment -

        One relevant argument, I have heard from Sanjay, is that if you are writing a PB client in C for HBase, then basically, you shouldn't be relying on the Java's semantics of exceptions. If you consider something like RegionMovedException, it is a case where the C client might want to inspect and do some action.

        Elliot, how would you suggest we pass the RegionMovedException from RegionServer through the ipc? The region server will throw an actual exception wrapping the message which will be understood by the ipc layer, or the region server does not throw the exception, but set the exception message to the response?

        Show
        Enis Soztutar added a comment - One relevant argument, I have heard from Sanjay, is that if you are writing a PB client in C for HBase, then basically, you shouldn't be relying on the Java's semantics of exceptions. If you consider something like RegionMovedException, it is a case where the C client might want to inspect and do some action. Elliot, how would you suggest we pass the RegionMovedException from RegionServer through the ipc? The region server will throw an actual exception wrapping the message which will be understood by the ipc layer, or the region server does not throw the exception, but set the exception message to the response?
        Hide
        stack added a comment -

        Maybe, have such exceptions implement an interface like PBException that has methods that would allow them to be ser/de.

        First, let me say, using exceptions for messaging is perverse as RegionMovedException is doing.

        Second, how many instances of payload carrying exceptions do we have? One, two?

        How would this work? Per Exception, we would define a pb Message (because we want to use pb serializing). You can't have a pb Message be an exception because it can't inherit, not unless we mess w/ protoc, nor can we subclass the pb since Throwable is a class, so, PBException would have serializeAsPB and deserializeAsPB methods in its Interface (sounds like Writable!).

        On serverside, when we get one of these messages, we would treat it different doing the above serializeAsPB. We'd then have an 'exception' pb Message. I would mark the header saying we have an exception and then I'd also write the classname into the response header.

        On client, would notice an exception then would see if a classname. If present, would create an instance or just invoke a static deserialize method that would know the pb to use and would undo the pb (can't have static methods in Interfaces so this route would be a little messy), then we would return an instance of aforementioned exception, wrap it in RemoteException and throw it?

        Sounds like a bit of work. Complicates things some too. If our rpc is simple, prospect of writing a non-java client would be less daunting.

        What you reckon?

        Show
        stack added a comment - Maybe, have such exceptions implement an interface like PBException that has methods that would allow them to be ser/de. First, let me say, using exceptions for messaging is perverse as RegionMovedException is doing. Second, how many instances of payload carrying exceptions do we have? One, two? How would this work? Per Exception, we would define a pb Message (because we want to use pb serializing). You can't have a pb Message be an exception because it can't inherit, not unless we mess w/ protoc, nor can we subclass the pb since Throwable is a class, so, PBException would have serializeAsPB and deserializeAsPB methods in its Interface (sounds like Writable!). On serverside, when we get one of these messages, we would treat it different doing the above serializeAsPB. We'd then have an 'exception' pb Message. I would mark the header saying we have an exception and then I'd also write the classname into the response header. On client, would notice an exception then would see if a classname. If present, would create an instance or just invoke a static deserialize method that would know the pb to use and would undo the pb (can't have static methods in Interfaces so this route would be a little messy), then we would return an instance of aforementioned exception, wrap it in RemoteException and throw it? Sounds like a bit of work. Complicates things some too. If our rpc is simple, prospect of writing a non-java client would be less daunting. What you reckon?
        Hide
        stack added a comment -

        Currently in RPC, there is an ExceptionMessage used to carry exceptions. It has, as you'd expect, exception name and stack trace. What if we added opaque Map, a repeatable PairByteBytes, to ExceptionMessage. On serialization, we'd looks for a Dictionary marker Interface. If present, would serialize the content. Ditto on deserialize. Would not be evolvable. You would have to have a different key name if you wanted to change format. Its generic though.

        Show
        stack added a comment - Currently in RPC, there is an ExceptionMessage used to carry exceptions. It has, as you'd expect, exception name and stack trace. What if we added opaque Map, a repeatable PairByteBytes, to ExceptionMessage. On serialization, we'd looks for a Dictionary marker Interface. If present, would serialize the content. Ditto on deserialize. Would not be evolvable. You would have to have a different key name if you wanted to change format. Its generic though.
        Hide
        Enis Soztutar added a comment -

        First, let me say, using exceptions for messaging is perverse as RegionMovedException is doing.

        That is why I am more convinced of the alternate proposal of having things like RME as valid return types from get/put/etc.

        Second, how many instances of payload carrying exceptions do we have? One, two?

        The most offending one I can think of is DoNotRetryIOException. By definition, you are passing some semantics (although no data) to the client as an exception by using the exception class hierarchy. Imagine again a C client, in which we get the actual exception class, but don't have the java class hierarchy to reason about whether this class inherits from DNRIOE.

        Show
        Enis Soztutar added a comment - First, let me say, using exceptions for messaging is perverse as RegionMovedException is doing. That is why I am more convinced of the alternate proposal of having things like RME as valid return types from get/put/etc. Second, how many instances of payload carrying exceptions do we have? One, two? The most offending one I can think of is DoNotRetryIOException. By definition, you are passing some semantics (although no data) to the client as an exception by using the exception class hierarchy. Imagine again a C client, in which we get the actual exception class, but don't have the java class hierarchy to reason about whether this class inherits from DNRIOE.
        Hide
        Elliott Clark added a comment - - edited

        The region server will throw an actual exception wrapping the message which will be understood by the ipc layer, or the region server does not throw the exception, but set the exception message to the response?

        I'm not sure I understand the question, so I might not answer the exact question you're asking. However here's my thinking:

        Right now the HRegionServer throws the exception and the ipc classes on the server side catch the exception. This exception is currently a message carrying two strings that're parsed on the client side.

        I'm proposing a that exception is sent as bytes + name of the class to decode these bytes. So a couple of examples:

        1. HRegionServer throws NoSuchColumFamilyException (could be any of the normal non payload carrying exceptions)
        2. The ipc handler catches the exception.
        3. Sees that there's no registered way to serialize this specially.
        4. So the ipc handler would use the default StringBasedException.
        5. That pb would be built and the bytes array would be added to the response along side the key that tells the client to decode on it's side using StringBasedException.
        6. The client would then get the message.
        7. See there is an exception.
        8. look up how to decode it using exceptionMessageType.
        9. decode the bytes array and throw the exception

        The special case:

        1. HRegionServer throws RegionMovedException
        2. The ipc classes catch the exception
        3. See that this is a specially registered class.
        4. Use the registered encode to take RegionMovedException and create RegionMovedExceptionMessage.
        5. Build the message
        6. put the bytes into the exception field.
        7. put the key telling the client to decode this as RegionMovedExceptionMessage into exceptionMessageType
        8. client would get the message
        9. see that there's an exception
        10. load the class the key says will decode the exception bytes.
        11. Use it to parse the RegionMovedExceptionMessage
        12. throw the exception on the client side.

        Edit: Formatting.

        Show
        Elliott Clark added a comment - - edited The region server will throw an actual exception wrapping the message which will be understood by the ipc layer, or the region server does not throw the exception, but set the exception message to the response? I'm not sure I understand the question, so I might not answer the exact question you're asking. However here's my thinking: Right now the HRegionServer throws the exception and the ipc classes on the server side catch the exception. This exception is currently a message carrying two strings that're parsed on the client side. I'm proposing a that exception is sent as bytes + name of the class to decode these bytes. So a couple of examples: HRegionServer throws NoSuchColumFamilyException (could be any of the normal non payload carrying exceptions) The ipc handler catches the exception. Sees that there's no registered way to serialize this specially. So the ipc handler would use the default StringBasedException. That pb would be built and the bytes array would be added to the response along side the key that tells the client to decode on it's side using StringBasedException. The client would then get the message. See there is an exception. look up how to decode it using exceptionMessageType. decode the bytes array and throw the exception The special case: HRegionServer throws RegionMovedException The ipc classes catch the exception See that this is a specially registered class. Use the registered encode to take RegionMovedException and create RegionMovedExceptionMessage. Build the message put the bytes into the exception field. put the key telling the client to decode this as RegionMovedExceptionMessage into exceptionMessageType client would get the message see that there's an exception load the class the key says will decode the exception bytes. Use it to parse the RegionMovedExceptionMessage throw the exception on the client side. Edit: Formatting.
        Hide
        stack added a comment -

        WIP

        Implement spec (sending header and then the request Message return response Message)

        Currently patch does too much. Will cut it back. For example:

        1. Under the proxy, it uses pb Service too. A Stub/Service
        is created when we create the proxy. We use the Stub/Service
        doing lookups to find request and response types as well as
        for making the invocations rather than user reflection. Should
        do this in a separate patch instead of in here dependent on
        whether we decide to move to pb Service rather than use
        Proxy/reflection.
        2. Removes QoSFunction priority setting. Used work by combination
        of annotations and inspection of regionname. Would undo request
        to figure it out. Awkward. Simplier (as per Elliott idea). is
        just having client set priority. Server can choose to respect
        priority or not. Saves a bunch of code.

        TODO: Need to get encodedatablocks into the mix. Currently not there.
        TODO: Add to IpcProtocol returning a Service Class. Will clean up implementation.
        TODO: This patch is not working yet.
        TODO: Add priority to client calls.

        Show
        stack added a comment - WIP Implement spec (sending header and then the request Message return response Message) Currently patch does too much. Will cut it back. For example: 1. Under the proxy, it uses pb Service too. A Stub/Service is created when we create the proxy. We use the Stub/Service doing lookups to find request and response types as well as for making the invocations rather than user reflection. Should do this in a separate patch instead of in here dependent on whether we decide to move to pb Service rather than use Proxy/reflection. 2. Removes QoSFunction priority setting. Used work by combination of annotations and inspection of regionname. Would undo request to figure it out. Awkward. Simplier (as per Elliott idea). is just having client set priority. Server can choose to respect priority or not. Saves a bunch of code. TODO: Need to get encodedatablocks into the mix. Currently not there. TODO: Add to IpcProtocol returning a Service Class. Will clean up implementation. TODO: This patch is not working yet. TODO: Add priority to client calls.
        Hide
        Devaraj Das added a comment -

        What I was thinking maps well to what Elliot just said (Elliot thanks for the detailed sequence). In general, I was trying to preserve exceptions, so that if methods are called inline (without going through RPC), they would still throw those exceptions. And if the client is Java, invoking the method over RPC, they also wouldn't need to change (but the RPC layer creates the appropriate exceptions and throws it up to the client call). For clients in C, they would get the exceptionName and the relevant bytes and yes, they'd need to do special handling.. (and I agree this is less than ideal for them).

        Show
        Devaraj Das added a comment - What I was thinking maps well to what Elliot just said (Elliot thanks for the detailed sequence). In general, I was trying to preserve exceptions, so that if methods are called inline (without going through RPC), they would still throw those exceptions. And if the client is Java, invoking the method over RPC, they also wouldn't need to change (but the RPC layer creates the appropriate exceptions and throws it up to the client call). For clients in C, they would get the exceptionName and the relevant bytes and yes, they'd need to do special handling.. (and I agree this is less than ideal for them).
        Hide
        stack added a comment -

        How many of these unorthodox payload-bearing exceptions do we have? Is it one? Two? Why not just add these to the spec and then the special handling for each rather than conjure an extensible system that has a registry apart from the one we already have, the pb ServiceDescriptor?

        Show
        stack added a comment - How many of these unorthodox payload-bearing exceptions do we have? Is it one? Two? Why not just add these to the spec and then the special handling for each rather than conjure an extensible system that has a registry apart from the one we already have, the pb ServiceDescriptor?
        Hide
        Enis Soztutar added a comment -

        I'm not sure I understand the question, so I might not answer the exact question you're asking. However here's my thinking:

        Thanks, I was asking about how are we going to pass the exception from the region server layer to the ipc layer without breaking layering. Not sure registering serializers for exceptions is the cleanest way though.
        I think what DD was talking about above, was something like this:

        class PBException extends Exception { 
        Message message;
        PBException(Message message) {this.message = message;}
        }
        class RegionMovedException extends PBException {
           RegionMovedException(RegionMovedData message) {super(message);} // ctor from server side
           RegionMovedException(byte[] bytes) {} // ctor from server side   
        }
        message RegionMovedData {
          required ServerName serverName = 1;
        }
        

        The flow would be:
        1. HRegionServer throws RegionMovedException
        2. The ipc classes catch the exception
        3. Checks exception is instanceof PBException. --> ipc knows about PBException
        4. Constructs the exceptionBody bytes by serializing ex.message
        5. put the bytes into the exception field.
        6. put the class name "RegionMovedException" as exception class name
        7. client would get the message
        8. see that there's an exception
        9. Instantiate and throw the exception class (RegionMovedException) by using the byte[] ctor, which would decode this as RegionMovedData

        Show
        Enis Soztutar added a comment - I'm not sure I understand the question, so I might not answer the exact question you're asking. However here's my thinking: Thanks, I was asking about how are we going to pass the exception from the region server layer to the ipc layer without breaking layering. Not sure registering serializers for exceptions is the cleanest way though. I think what DD was talking about above, was something like this: class PBException extends Exception { Message message; PBException(Message message) { this .message = message;} } class RegionMovedException extends PBException { RegionMovedException(RegionMovedData message) { super (message);} // ctor from server side RegionMovedException( byte [] bytes) {} // ctor from server side } message RegionMovedData { required ServerName serverName = 1; } The flow would be: 1. HRegionServer throws RegionMovedException 2. The ipc classes catch the exception 3. Checks exception is instanceof PBException. --> ipc knows about PBException 4. Constructs the exceptionBody bytes by serializing ex.message 5. put the bytes into the exception field. 6. put the class name "RegionMovedException" as exception class name 7. client would get the message 8. see that there's an exception 9. Instantiate and throw the exception class (RegionMovedException) by using the byte[] ctor, which would decode this as RegionMovedData
        Hide
        stack added a comment -

        Enis Soztutar Yeah. Looks like Elliott's scratches but rather than a factory you'd just drop the bytes into the Constructor and let the class make sense of it. That'd work.

        But I'd suggest we do not need a generically extensible system that allows adding new payload-bearing exception types (see above where we would want to discourage control via exception).

        Reading through RegionServer exceptions I see one exception that takes payload, RegionServerMoved, so let us just explicitly handle this type beyond the base 'String' type we already have in rpc.

        Do you lot think there could be more? It would have to be exceptions that were not method specific because for these types then we should suggest the method response include the exception as an optional response. So that leaves exceptions that could come out of multiple methods or problems the server encounters while running.

        Chatting w/ Elliott, the base 'String' Exception type is already extensible being a protobuf so we could add extra info therein that the client exception classes could interpret if able such as how long to hold before retrying (a pushback).

        Can you lads think of any others?

        I'd think that we'd try and keep the spec as narrow as possible (and no narrower) so if we could do without having an extensible fancy payload bearing exception types system, lets punt.

        Show
        stack added a comment - Enis Soztutar Yeah. Looks like Elliott's scratches but rather than a factory you'd just drop the bytes into the Constructor and let the class make sense of it. That'd work. But I'd suggest we do not need a generically extensible system that allows adding new payload-bearing exception types (see above where we would want to discourage control via exception). Reading through RegionServer exceptions I see one exception that takes payload, RegionServerMoved, so let us just explicitly handle this type beyond the base 'String' type we already have in rpc. Do you lot think there could be more? It would have to be exceptions that were not method specific because for these types then we should suggest the method response include the exception as an optional response. So that leaves exceptions that could come out of multiple methods or problems the server encounters while running. Chatting w/ Elliott, the base 'String' Exception type is already extensible being a protobuf so we could add extra info therein that the client exception classes could interpret if able such as how long to hold before retrying (a pushback). Can you lads think of any others? I'd think that we'd try and keep the spec as narrow as possible (and no narrower) so if we could do without having an extensible fancy payload bearing exception types system, lets punt.
        Hide
        Enis Soztutar added a comment -

        But I'd suggest we do not need a generically extensible system that allows adding new payload-bearing exception types (see above where we would want to discourage control via exception).

        Totally agreed.

        Do you lot think there could be more? It would have to be exceptions that were not method specific because for these types then we should suggest the method response include the exception as an optional response. So that leaves exceptions that could come out of multiple methods or problems the server encounters while running.

        Again, the other concerning thing is DoNotRetryIOException. Although it does not carry data, we basically do control flow on subclasses of this exception. Maybe we should add a field bool doNotRetry, in the RpcException.

        I'd think that we'd try and keep the spec as narrow as possible (and no narrower) so if we could do without having an extensible fancy payload bearing exception types system, lets punt.

        Agreed. But what I am saying is that we already do have those in place and get rid of the string parsing one way or the other.

        Show
        Enis Soztutar added a comment - But I'd suggest we do not need a generically extensible system that allows adding new payload-bearing exception types (see above where we would want to discourage control via exception). Totally agreed. Do you lot think there could be more? It would have to be exceptions that were not method specific because for these types then we should suggest the method response include the exception as an optional response. So that leaves exceptions that could come out of multiple methods or problems the server encounters while running. Again, the other concerning thing is DoNotRetryIOException. Although it does not carry data, we basically do control flow on subclasses of this exception. Maybe we should add a field bool doNotRetry, in the RpcException. I'd think that we'd try and keep the spec as narrow as possible (and no narrower) so if we could do without having an extensible fancy payload bearing exception types system, lets punt. Agreed. But what I am saying is that we already do have those in place and get rid of the string parsing one way or the other.
        Hide
        Ted Yu added a comment -

        RegionMovedException is new in 0.96
        Without RegionMovedException, we would lose some benefit in HConnectionImplementation.updateCachedLocations() but would have better formation for RPC spec.

        Show
        Ted Yu added a comment - RegionMovedException is new in 0.96 Without RegionMovedException, we would lose some benefit in HConnectionImplementation.updateCachedLocations() but would have better formation for RPC spec.
        Hide
        stack added a comment -

        Maybe we should add a field bool doNotRetry, in the RpcException.

        Let me add this flag.

        But what I am saying is that we already do have those in place and get rid of the string parsing one way or the other.

        So just do base stringify exception handling and RegionMovedException? (And any other we turn up before 0.96 release)? And leave at that?

        If so, good by me and I'll write it into spec. Thanks.

        Show
        stack added a comment - Maybe we should add a field bool doNotRetry, in the RpcException. Let me add this flag. But what I am saying is that we already do have those in place and get rid of the string parsing one way or the other. So just do base stringify exception handling and RegionMovedException? (And any other we turn up before 0.96 release)? And leave at that? If so, good by me and I'll write it into spec. Thanks.
        Hide
        Ted Yu added a comment -

        Both RegionMovedException and DoNotRetryIOException give clients hint on what to do next.
        I think their representation in .proto can be unified.

        class RegionRelocationException extends PBException {
          // ctor from server side
          RegionRelocationException(RegionRelocationData message) {
            super(message);
          }
          RegionRelocationException(byte[] bytes) {
          } // ctor from server side   
        }
        message RegionRelocationData {
          required ServerName serverName = 1;
        }
        

        We can designate a special value for RegionRelocationData.serverName which tells client that client should not retry.

        Show
        Ted Yu added a comment - Both RegionMovedException and DoNotRetryIOException give clients hint on what to do next. I think their representation in .proto can be unified. class RegionRelocationException extends PBException { // ctor from server side RegionRelocationException(RegionRelocationData message) { super (message); } RegionRelocationException( byte [] bytes) { } // ctor from server side } message RegionRelocationData { required ServerName serverName = 1; } We can designate a special value for RegionRelocationData.serverName which tells client that client should not retry.
        Hide
        stack added a comment -

        You would put RegionMovedException and DoNotRetryIOException together into a new Exception named RegionRelocationException?

        I don't follow how not retrying is related to Region Relocation.

        ... We can designate a special value for RegionRelocationData.serverName which tells client that client should not retry.

        The above seems a little cryptic, don't you think?

        Unless I hear otherwise, I'll add a do not retry flag to our base stringified exception Message (might as well add ServerName to the base Message too – c/c++s might be able to make sense of it). Will also add special case RegionMovedException pulling out the hostname and port whenever I see one of these...

        Show
        stack added a comment - You would put RegionMovedException and DoNotRetryIOException together into a new Exception named RegionRelocationException? I don't follow how not retrying is related to Region Relocation. ... We can designate a special value for RegionRelocationData.serverName which tells client that client should not retry. The above seems a little cryptic, don't you think? Unless I hear otherwise, I'll add a do not retry flag to our base stringified exception Message (might as well add ServerName to the base Message too – c/c++s might be able to make sense of it). Will also add special case RegionMovedException pulling out the hostname and port whenever I see one of these...
        Hide
        Jonathan Hsieh added a comment -

        stack can you enable comment rights on the doc?

        Show
        Jonathan Hsieh added a comment - stack can you enable comment rights on the doc?
        Hide
        stack added a comment -

        Done for you. If anyone else wants access, just ask. Thanks.

        Show
        stack added a comment - Done for you. If anyone else wants access, just ask. Thanks.
        Hide
        stack added a comment -

        Redo to match spec. Let me do edit pass on spec. and run this through tests then put up a patch on rb.

        On patch, we used to pass in a fat pb object that was most of the header info and then included the pb request param. Now we pass header and param separately. Thats the spec.

        Adds comments and some method and data member renaming to better match spec and pb.

        Because more of protocol in pb and because an rpc request is made of a pb header and the already made pb param – e.g. GetRequest, etc. – and because we pass exceptions as pb, etc., HBaseServer and HBaseClient has more pb in it now; e.g. assembling the headers to make requests, undoing responses.

        If error, the pb'd exception carries server/port if regionmoved and it is marked do not retry if exception is such.

        Remove dead code: ParallelCall, ParallelReturn

        Added IPCUtil, FatalConnectionException

        Moved internal classes out of HBaseServer and HRegionServer.

        Getting the encoded datablocks the spec talks of in there is a todo as is pb'ing the kv and work to cut down on buffer copies (the latter is a separate issue).

        Show
        stack added a comment - Redo to match spec. Let me do edit pass on spec. and run this through tests then put up a patch on rb. On patch, we used to pass in a fat pb object that was most of the header info and then included the pb request param. Now we pass header and param separately. Thats the spec. Adds comments and some method and data member renaming to better match spec and pb. Because more of protocol in pb and because an rpc request is made of a pb header and the already made pb param – e.g. GetRequest, etc. – and because we pass exceptions as pb, etc., HBaseServer and HBaseClient has more pb in it now; e.g. assembling the headers to make requests, undoing responses. If error, the pb'd exception carries server/port if regionmoved and it is marked do not retry if exception is such. Remove dead code: ParallelCall, ParallelReturn Added IPCUtil, FatalConnectionException Moved internal classes out of HBaseServer and HRegionServer. Getting the encoded datablocks the spec talks of in there is a todo as is pb'ing the kv and work to cut down on buffer copies (the latter is a separate issue).
        Hide
        Elliott Clark added a comment -

        I would vote moving exception into the Header and adding a boolean for contains response. That would allow returning partial requests if there was a timeout or other error (feature for 0.98++). Other than that everything looks good.

        Show
        Elliott Clark added a comment - I would vote moving exception into the Header and adding a boolean for contains response. That would allow returning partial requests if there was a timeout or other error (feature for 0.98++). Other than that everything looks good.
        Hide
        Elliott Clark added a comment -

        After a little more time to look at things and discuss here are a few more thoughts:

        Lets remove the varint infront of ResquestBody and Response body and instead put that data into the header (something like RequestBodyMeta and ResponseBodyMeta). That makes it more in line with how ecd's will work. If there's a meta object, for body or ecd, then look in the meta for size and pull that off the wire.

        I would also think that the total request size infront of request and response could be removed. That data is duplicated. totalRequesSize = headerSizeVarInt + headerSize + bodySize + ecdSize. That's a larger change but gets us closer to being more async.

        Show
        Elliott Clark added a comment - After a little more time to look at things and discuss here are a few more thoughts: Lets remove the varint infront of ResquestBody and Response body and instead put that data into the header (something like RequestBodyMeta and ResponseBodyMeta). That makes it more in line with how ecd's will work. If there's a meta object, for body or ecd, then look in the meta for size and pull that off the wire. I would also think that the total request size infront of request and response could be removed. That data is duplicated. totalRequesSize = headerSizeVarInt + headerSize + bodySize + ecdSize. That's a larger change but gets us closer to being more async.
        Hide
        Ted Yu added a comment -

        ecd stands for encoded datablocks in the above description, right ?

        Show
        Ted Yu added a comment - ecd stands for encoded datablocks in the above description, right ?
        Hide
        Elliott Clark added a comment -

        Correct, ECD is encoded data blocks. Sorry I should have been more clear.

        Show
        Elliott Clark added a comment - Correct, ECD is encoded data blocks. Sorry I should have been more clear.
        Hide
        stack added a comment -

        Thanks for review Elliott Clark especially given I have yet to update doc to match code. Suggestions sound good to me. Regards the total length up front, let me look at changing it so the int is just length of header (but not a varint) and as you suggest, get the param and possible EDB lengths out of the header pb (will do ditto on the response – trying to keep response mirror image or request)

        Show
        stack added a comment - Thanks for review Elliott Clark especially given I have yet to update doc to match code. Suggestions sound good to me. Regards the total length up front, let me look at changing it so the int is just length of header (but not a varint) and as you suggest, get the param and possible EDB lengths out of the header pb (will do ditto on the response – trying to keep response mirror image or request)
        Hide
        Devaraj Das added a comment -

        Looks good overall. I think most of what I was planning to fix in HBASE-5945 has been taken into consideration in this one.

        Couple of comments:
        1. Why is the callId made a long? Is it for future proofing?
        2. The exception handling seems somewhat customized. Maybe it's fine but it stands out. Will see if I can think of a better approach short of the full blown exception handling in RPC (which you probably don't like ).

        • the special casing of the RegionMovedException
        • RemoteWithExtrasException
          I ran some RPC tests (TestDelayedRpc, TestProtoBufRpc) with the patch. They passed. TestPriorityRpc failed. When I replaced isMetaTable calls with isMetaRegion (as in the earlier code), it passed as well.

        Are we breaking this up into two parts - one this, and another for the encoded data blocks thing?

        Show
        Devaraj Das added a comment - Looks good overall. I think most of what I was planning to fix in HBASE-5945 has been taken into consideration in this one. Couple of comments: 1. Why is the callId made a long? Is it for future proofing? 2. The exception handling seems somewhat customized. Maybe it's fine but it stands out. Will see if I can think of a better approach short of the full blown exception handling in RPC (which you probably don't like ). the special casing of the RegionMovedException RemoteWithExtrasException I ran some RPC tests (TestDelayedRpc, TestProtoBufRpc) with the patch. They passed. TestPriorityRpc failed. When I replaced isMetaTable calls with isMetaRegion (as in the earlier code), it passed as well. Are we breaking this up into two parts - one this, and another for the encoded data blocks thing?
        Hide
        stack added a comment -

        Devaraj Das

        Thanks for review.

        On callid, I'll probably put it back to int in final patch just to make the diff smaller.

        On exception handling, rather than build a framework to support exceptions-with-any-arbitrary-payload, instead, just do the handling of the one case where we know there is payload – RegionMovedException. RemoteWithExtrasException is just that, the hadoop RemoteException w/ extra info that can be exploited or not). I don't think it too bad really. We have this one extensible ExceptionResponse that we can throw any info we need to pass into. On client, if it supports the extra info, it can do the special casing for whatever the new type or just shove the info into RemoteWithExtrasException.

        If you have ideas on how to make it cleaner, much appreciated.

        Regards the uploaded patch, it is stale now. I want to incorporate Elliott comments. I spent some time on it over the weekend. The upfront total length of the request and response that Elliott suggested we punt is unavoidable it seems, unless we rewrite the server-side. The server has a loop which reads ints and dependent on int content and its current processing state, pulls in the preamble, reads the connection header, or reads in the WHOLE request. The server is about bytebuffers gotten off the server socket channel and though it is technically async, the way the server is written, it expects to be able to pull all the bytes that make up the request in one giant sucking read. Though it gives the impression that you could 'park' the read and continue it later if all bytes are net yet present on the wire, that facility is not hooked up.

        It would be sweet being able to park the read or just read the header and if not high priority, come back for the request part latter but I'm thinking that is v2 of the rpc protocol when we go full async on the server side.

        Regards hbase-5945, pbs are worth study. Unless you pass an explicit byte array size for the Message, pb will under the wrappers instantiate a Coded*Stream whose construction will allocate a 4k buffer which we do not want. I was messing trying to figure the marshalling/unmarshalling path that does least copying and allocations and looking at pb 2.5.0, as yet unreleased, which claims its new parse method 25% faster...

        I"ll be back.

        Show
        stack added a comment - Devaraj Das Thanks for review. On callid, I'll probably put it back to int in final patch just to make the diff smaller. On exception handling, rather than build a framework to support exceptions-with-any-arbitrary-payload, instead, just do the handling of the one case where we know there is payload – RegionMovedException. RemoteWithExtrasException is just that, the hadoop RemoteException w/ extra info that can be exploited or not). I don't think it too bad really. We have this one extensible ExceptionResponse that we can throw any info we need to pass into. On client, if it supports the extra info, it can do the special casing for whatever the new type or just shove the info into RemoteWithExtrasException. If you have ideas on how to make it cleaner, much appreciated. Regards the uploaded patch, it is stale now. I want to incorporate Elliott comments. I spent some time on it over the weekend. The upfront total length of the request and response that Elliott suggested we punt is unavoidable it seems, unless we rewrite the server-side. The server has a loop which reads ints and dependent on int content and its current processing state, pulls in the preamble, reads the connection header, or reads in the WHOLE request. The server is about bytebuffers gotten off the server socket channel and though it is technically async, the way the server is written, it expects to be able to pull all the bytes that make up the request in one giant sucking read. Though it gives the impression that you could 'park' the read and continue it later if all bytes are net yet present on the wire, that facility is not hooked up. It would be sweet being able to park the read or just read the header and if not high priority, come back for the request part latter but I'm thinking that is v2 of the rpc protocol when we go full async on the server side. Regards hbase-5945, pbs are worth study. Unless you pass an explicit byte array size for the Message, pb will under the wrappers instantiate a Coded*Stream whose construction will allocate a 4k buffer which we do not want. I was messing trying to figure the marshalling/unmarshalling path that does least copying and allocations and looking at pb 2.5.0, as yet unreleased, which claims its new parse method 25% faster... I"ll be back.
        Hide
        Devaraj Das added a comment -

        +1 on having a follow up jira to really do the streaming (without passing the length apriori).

        On the HBASE-5945, I agree on the use of Coded*Stream (and the performance improvements in PB 2.5 seems interesting). In the patches posted on 5945, Coded*Stream has been taken into account. But yes, once this issue is resolved, we should concentrate on such things in 5945.

        Thanks stack.

        Show
        Devaraj Das added a comment - +1 on having a follow up jira to really do the streaming (without passing the length apriori). On the HBASE-5945 , I agree on the use of Coded*Stream (and the performance improvements in PB 2.5 seems interesting). In the patches posted on 5945, Coded*Stream has been taken into account. But yes, once this issue is resolved, we should concentrate on such things in 5945. Thanks stack .
        Hide
        stack added a comment -

        Thanks again for review DD.

        Let me attach the latest patch w/ Elliott's comments accomodated and some new stuff....

        Show
        stack added a comment - Thanks again for review DD. Let me attach the latest patch w/ Elliott's comments accomodated and some new stuff....
        Hide
        stack added a comment -

        Refactor in RPC.proto to accomodate Elliott feedback. See diff for description of the protocol.
        Trying to miniize change. Not too different from previous patches. Just has more failed unit
        tests fixed.

        Still missing is the TODO EncodedDataBlock though at Matt Corgan suggestion and looking back at
        my old patch in HBASE-7233, Seializing KeyValues, this instead will just be bytes produced by
        a CellOutputStream and then read with a CellScanner.java – two simple Cell Interfaces). Let
        me try and get a basic implementation in and then call this patch quits.

        Now if you enable ipc logging you see the complete client side request and over on server
        the complete receive and then what it writes on the wire. Its over the top but nice debugging.
        It is only on if you enable debug. Will work on a custom pb TextFormatter that doesn't output
        it all – that has some bounds – after this patch goes in. Need to make it so this toString
        shows up in the UI to where we show process listing.

        Renamed some methods in rpc so matches spec and so its clearer whats going on in here.

        Effort to avoid buffer creations and oversized buffer creations by pb.

        Removed unused classes // Call and moved some inner classes out to be package protected
        standalones such as QosFunction and MethodCache. This is the bulk of the patch.

        Show
        stack added a comment - Refactor in RPC.proto to accomodate Elliott feedback. See diff for description of the protocol. Trying to miniize change. Not too different from previous patches. Just has more failed unit tests fixed. Still missing is the TODO EncodedDataBlock though at Matt Corgan suggestion and looking back at my old patch in HBASE-7233 , Seializing KeyValues, this instead will just be bytes produced by a CellOutputStream and then read with a CellScanner.java – two simple Cell Interfaces). Let me try and get a basic implementation in and then call this patch quits. Now if you enable ipc logging you see the complete client side request and over on server the complete receive and then what it writes on the wire. Its over the top but nice debugging. It is only on if you enable debug. Will work on a custom pb TextFormatter that doesn't output it all – that has some bounds – after this patch goes in. Need to make it so this toString shows up in the UI to where we show process listing. Renamed some methods in rpc so matches spec and so its clearer whats going on in here. Effort to avoid buffer creations and oversized buffer creations by pb. Removed unused classes // Call and moved some inner classes out to be package protected standalones such as QosFunction and MethodCache. This is the bulk of the patch.
        Hide
        stack added a comment -

        Some other notes.

        In this patch, request, response, connection header, etc., all are preceeded by a total length int. It is handy and a little redundant but removing it would require a refactor of server-side which I don't want to do (see comments above). Security would also become interesting if we do other than the current one big pull and one big write on the server-side. So, current server-side implementation ends up dictating our on-the-wire format some.

        Show
        stack added a comment - Some other notes. In this patch, request, response, connection header, etc., all are preceeded by a total length int. It is handy and a little redundant but removing it would require a refactor of server-side which I don't want to do (see comments above). Security would also become interesting if we do other than the current one big pull and one big write on the server-side. So, current server-side implementation ends up dictating our on-the-wire format some.
        Hide
        stack added a comment -

        Try against Jenkins for kicks.

        Show
        stack added a comment - Try against Jenkins for kicks.
        Hide
        Devaraj Das added a comment -

        Hadoopqa didn't get triggered on the last upload. Uploading Stack's patch again.

        BTW Stack do you want to open a RB on this.

        Show
        Devaraj Das added a comment - Hadoopqa didn't get triggered on the last upload. Uploading Stack's patch again. BTW Stack do you want to open a RB on this.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12568064/rpc_spec3.txt
        against trunk revision .

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

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

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/4340//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/12568064/rpc_spec3.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 9 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/4340//console This message is automatically generated.
        Hide
        stack added a comment -

        Thanks Devaraj Das. Looks like it doesn't apply anymore. Let me upload a fixup soon. Will put it up on rb when I have something committable boss. Thanks.

        Show
        stack added a comment - Thanks Devaraj Das . Looks like it doesn't apply anymore. Let me upload a fixup soon. Will put it up on rb when I have something committable boss. Thanks.
        Hide
        Ted Yu added a comment -

        For QosFunction#apply():

        +      if (region.getRegionInfo().isMetaRegion()) {
        ...
        +      if (scanner != null && scanner.getRegionInfo().isMetaRegion()) {
        

        isMetaTable() should be called above.

        Show
        Ted Yu added a comment - For QosFunction#apply(): + if (region.getRegionInfo().isMetaRegion()) { ... + if (scanner != null && scanner.getRegionInfo().isMetaRegion()) { isMetaTable() should be called above.
        Hide
        stack added a comment -

        Made this a subtask of hbase-7898. That issue is about rpc. Code in that issue is implementing this spec.

        Show
        stack added a comment - Made this a subtask of hbase-7898. That issue is about rpc. Code in that issue is implementing this spec.
        Hide
        stack added a comment -

        Here is an appendix for the refguide. Its a spec for our 0.95 wire format. Will just go ahead and commit it. Can be reviewed post commit after its up on website – easier to review.

        Show
        stack added a comment - Here is an appendix for the refguide. Its a spec for our 0.95 wire format. Will just go ahead and commit it. Can be reviewed post commit after its up on website – easier to review.
        Hide
        stack added a comment -

        The patch is a docbook version of the google doc https://docs.google.com/document/d/1-1RJMLXzYldmHgKP7M7ynK6euRpucD03fZ603DlZfGI/edit which I did a once-over on just now to make it match what code does.

        Show
        stack added a comment - The patch is a docbook version of the google doc https://docs.google.com/document/d/1-1RJMLXzYldmHgKP7M7ynK6euRpucD03fZ603DlZfGI/edit which I did a once-over on just now to make it match what code does.
        Hide
        stack added a comment -

        Committed to trunk. Will commit the refguide to 0.95 just before rc.

        Show
        stack added a comment - Committed to trunk. Will commit the refguide to 0.95 just before rc.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #3979 (See https://builds.apache.org/job/HBase-TRUNK/3979/)
        HBASE-7533 Write an RPC Specification for 0.96 (Revision 1459094)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/src/docbkx/book.xml
        • /hbase/trunk/src/docbkx/rpc.xml
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #3979 (See https://builds.apache.org/job/HBase-TRUNK/3979/ ) HBASE-7533 Write an RPC Specification for 0.96 (Revision 1459094) Result = FAILURE stack : Files : /hbase/trunk/src/docbkx/book.xml /hbase/trunk/src/docbkx/rpc.xml
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #456 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/456/)
        HBASE-7533 Write an RPC Specification for 0.96 (Revision 1459094)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/src/docbkx/book.xml
        • /hbase/trunk/src/docbkx/rpc.xml
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #456 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/456/ ) HBASE-7533 Write an RPC Specification for 0.96 (Revision 1459094) Result = FAILURE stack : Files : /hbase/trunk/src/docbkx/book.xml /hbase/trunk/src/docbkx/rpc.xml
        Hide
        stack added a comment -

        Addendum that comes of review comments made by Jon Hsieh up on the google docs (thanks Jon).

        Show
        stack added a comment - Addendum that comes of review comments made by Jon Hsieh up on the google docs (thanks Jon).
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #3982 (See https://builds.apache.org/job/HBase-TRUNK/3982/)
        HBASE-7533 Write an RPC Specification for 0.96; ADDENDUM (Revision 1459446)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/src/docbkx/rpc.xml
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #3982 (See https://builds.apache.org/job/HBase-TRUNK/3982/ ) HBASE-7533 Write an RPC Specification for 0.96; ADDENDUM (Revision 1459446) Result = FAILURE stack : Files : /hbase/trunk/src/docbkx/rpc.xml
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #458 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/458/)
        HBASE-7533 Write an RPC Specification for 0.96; ADDENDUM (Revision 1459446)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/src/docbkx/rpc.xml
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #458 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/458/ ) HBASE-7533 Write an RPC Specification for 0.96; ADDENDUM (Revision 1459446) Result = FAILURE stack : Files : /hbase/trunk/src/docbkx/rpc.xml
        Hide
        Enis Soztutar added a comment -

        Stack, the links seem broken. (http://hbase.apache.org/book/apjs03.html).

        Show
        Enis Soztutar added a comment - Stack, the links seem broken. ( http://hbase.apache.org/book/apjs03.html ).
        Hide
        stack added a comment -

        Marking closed.

        Show
        stack added a comment - Marking closed.

          People

          • Assignee:
            stack
            Reporter:
            stack
          • Votes:
            0 Vote for this issue
            Watchers:
            14 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development