Skip to content

Commit

Permalink
BOOKKEEPER-1018: Allow client to select older V2 protocol (no protobuf)
Browse files Browse the repository at this point in the history 
Originally done by Matteo Merli (merlimat). Tagging sijie and eolivelli  for review.

Author: Govind Menon <govindappumenon@gmail.com>

Reviewers: Sijie Guo <sijie@apache.org>

Closes #120 from govind-menon/BOOKKEEPER-1018
  • Loading branch information
@govind-menon @sijie
govind-menon authored and sijie committed Apr 3, 2017
1 parent 9836c87 commit 9001e30
Show file tree
Hide file tree
Showing 4 changed files with 336 additions and 134 deletions.
23 changes: 22 additions & 1 deletion bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public class ClientConfiguration extends AbstractConfiguration {
protected final static String CLIENT_WRITEBUFFER_HIGH_WATER_MARK = "clientWriteBufferHighWaterMark";
protected final static String CLIENT_CONNECT_TIMEOUT_MILLIS = "clientConnectTimeoutMillis";
protected final static String NUM_CHANNELS_PER_BOOKIE = "numChannelsPerBookie";

protected final static String USE_V2_WIRE_PROTOCOL = "useV2WireProtocol";
// Read Parameters
protected final static String READ_TIMEOUT = "readTimeout";
protected final static String SPECULATIVE_READ_TIMEOUT = "speculativeReadTimeout";
Expand Down Expand Up @@ -435,6 +435,27 @@ public ClientConfiguration setNumChannelsPerBookie(int numChannelsPerBookie) {
return this;
}

/**
* Use older Bookkeeper wire protocol (no protobuf)
*
* @return whether or not to use older Bookkeeper wire protocol (no protobuf)
*/
public boolean getUseV2WireProtocol() {
return getBoolean(USE_V2_WIRE_PROTOCOL, false);
}

/**
* Set whether or not to use older Bookkeeper wire protocol (no protobuf)
*
* @param useV2WireProtocol
* whether or not to use older Bookkeeper wire protocol (no protobuf)
* @return client configuration.
*/
public ClientConfiguration setUseV2WireProtocol(boolean useV2WireProtocol) {
setProperty(USE_V2_WIRE_PROTOCOL, useV2WireProtocol);
return this;
}

/**
* Get zookeeper servers to connect
*
Expand Down
8 changes: 8 additions & 0 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/AuthHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,14 @@ public void writeRequested(ChannelHandlerContext ctx,
} else {
waitingForAuth.add(e);
}
} else if (e.getMessage() instanceof BookieProtocol.Request) {
// let auth messages through, queue the rest
BookieProtocol.Request req = (BookieProtocol.Request)e.getMessage();
if (BookkeeperProtocol.OperationType.AUTH.getNumber() == req.getOpCode()) {
super.writeRequested(ctx, e);
} else {
waitingForAuth.add(e);
}
} // else just drop
}
}
Expand Down
3 changes: 3 additions & 0 deletions bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieProtocol.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,9 @@ short getFlags() {
* by the auth providers themselves.
*/
public static final byte AUTH = 3;
public static final byte READ_LAC = 4;
public static final byte WRITE_LAC = 5;
public static final byte GET_BOOKIE_INFO = 6;

/**
* The error code that indicates success
Expand Down

6 comments on commit 9001e30

@eolivelli
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for my very late reply

Some minor comments, maybe for future Jiras:
It seems to me or there is no testcase? How can we assert that all is workibg as expected?
The patch is quite straightforward but I'm worried about future changes. For instance the SSL patch.

@govind-menon . What is your use case? Do you have to connect to legacy bookies?

@sijie
Copy link
Member

@sijie sijie commented on 9001e30 Apr 3, 2017

Choose a reason for hiding this comment

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

@eolivelli I knew a bit history about this. @merlimat mentioned to me that yahoo is using v2 protocol, since v2 protocol is more friendly with jvm gc.

@merlimat can you chime in when you have time?

@merlimat
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with the protobuf based protocol is that it creates a lot of throw-away objects for each serialization/deserialization cycle (that also is multiplied by the number of bookies a particular client is writing).
The bigger issue though, is that with protobuf the payload gets copied and allocated from JVM heap multiple times. That's kind of the design of protobuf, to be safe, but it comes at a high cost.

By using the v2 binary protocol, we can make the client and bookie avoid any payload copy and also perform serialization/deserialization with 0 allocated objects.

@govind-menon
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eolivelli Thank you for your comments - I will add a test case.

@sijie Thanks for the review and merge

@merlimat
Copy link
Contributor

Choose a reason for hiding this comment

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

@govind-menon probably it would be good to just add a parameter so that existing tests are running on both protocols.

@govind-menon
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@merlimat Will do.

Please sign in to comment.