Skip to content
This repository has been archived by the owner on Mar 26, 2020. It is now read-only.

Cross GlusterD interaction using pbrpc #63

Merged
merged 11 commits into from
Jan 12, 2016
Merged

Conversation

atinmu
Copy link
Contributor

@atinmu atinmu commented Dec 30, 2015

No description provided.

Atin Mukherjee added 2 commits December 29, 2015 18:59
Signed-off-by: Atin Mukherjee <amukherj@redhat.com>
Signed-off-by: Atin Mukherjee <amukherj@redhat.com>
Signed-off-by: Atin Mukherjee <amukherj@redhat.com>
@atinmu atinmu changed the title R Cross GlusterD interaction using pbrpc Dec 30, 2015
Signed-off-by: Atin Mukherjee <amukherj@redhat.com>
"github.com/kshlm/pbrpc/pbcodec"
)

func (r *Connection) PeerAddRPCSvc(args *peer.PeerAddRequest, reply *RPCPeerAddResp) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error is here. You should be the protobuf generated type RPCPeerAddReq. You changed it in all other places, except this.

Because of this, ReadRequestBody in pbcodec isn't able to unmarshal the request body, and returns an error. The net/rpc framework silently ignores this and doesn't log or pass up any errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, silly! I corrected it. Now that its hitting the server handler but still there is a deadlock, client is not get back the response :(

@kshlm
Copy link
Member

kshlm commented Dec 30, 2015

This is not the structure I expected for RPC. The GD2 RPC package will be a general RPC package, which will be mostly a wrapper around net/rpc using pbcodec and the GD2 connection information. It would also provide methods for registering services. This package would be analogous to the /rpc/ directory in GlusterFS. The actual services would be implemented else-where, again as in GlusterFS.

I would suggest the following directory/package structuring.

rpc/
    server/ -> would contain the code necessary to create and manage a listener and methods to register services
    client/ -> would contain code necessary to establish and manage client connections and methods to send requests
    services/ -> would contain the actual services. The services and the handler functions would be defined here

func (r *Connection) PeerAddRPCSvc(args *peer.PeerAddRequest, reply *RPCPeerAddResp) error {
log.Debug("In PeerAdd")
if context.MaxOpVersion < 40000 {
*reply.OpRet = -1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reply.OpRet and reply.OpError are nil pointers, pointing to nothing. There is actually no location for the data you are setting to be stored. This is a side effect of using protobuf. Protobuf generated structs use only pointers as members. net/rpc package will allocate the struct and initialize the pointers to nil. The pointers will not be pointing to any allocated memory. You have to set these pointers to data you've allocated.

var (
  opRet int32
  opError string
)

// Update opRet and opError instead of reply.OpRet and reply.OpError

// before returning
reply.OpRet = &opRet
reply.OpError = &opError

return nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that did the trick! Many thanks @kshlm

Atin Mukherjee added 6 commits December 31, 2015 12:17
Signed-off-by: Atin Mukherjee <amukherj@redhat.com>
Signed-off-by: Atin Mukherjee <amukherj@redhat.com>
Signed-off-by: Atin Mukherjee <amukherj@redhat.com>
Signed-off-by: Atin Mukherjee <amukherj@redhat.com>
Signed-off-by: Atin Mukherjee <amukherj@redhat.com>
Signed-off-by: Atin Mukherjee <amukherj@redhat.com>
@kshlm
Copy link
Member

kshlm commented Jan 11, 2016

Looks good now.

You have vim swap file committed, please remove it.

Also, it'd be nice if the listener port were configurable. At least add a TODO for this.

Signed-off-by: Atin Mukherjee <amukherj@redhat.com>
atinmu pushed a commit that referenced this pull request Jan 12, 2016
Cross GlusterD interaction using pbrpc
@atinmu atinmu merged commit 40bfcc2 into gluster:master Jan 12, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants