bbeaudreault commented on code in PR #5366:
URL: https://github.com/apache/hbase/pull/5366#discussion_r1302171333
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerRpcConnection.java:
##########
@@ -405,6 +410,16 @@ private CodedInputStream createCis(ByteBuff buf) {
// Reads the connection header following version
private void processConnectionHeader(ByteBuff buf) throws IOException {
this.connectionHeader = ConnectionHeader.parseFrom(createCis(buf));
+ if (connectionHeader.getAttributeList().isEmpty()) {
Review Comment:
can you add a comment here that we want to copy the attributes prior to
releasing the buffer so that they don't get corrupted down the line when the
buffer's underlying memory is replaced for some other call?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerCall.java:
##########
@@ -209,8 +213,24 @@ public RequestHeader getHeader() {
}
@Override
- public RPCProtos.ConnectionHeader getConnectionHeader() {
- return this.connection.connectionHeader;
+ public Map<String, byte[]> getConnectionAttributes() {
+ return this.connection.connectionAttributes;
+ }
+
+ @Override
+ public Map<String, byte[]> getRequestAttributes() {
+ if (this.requestAttributes == null) {
+ if (header.getAttributeList().isEmpty()) {
+ this.requestAttributes = Collections.emptyMap();
+ } else {
+ this.requestAttributes =
Maps.newHashMapWithExpectedSize(header.getAttributeList().size());
Review Comment:
I don't think we need to synchronize this method given the chances of
threading issues (low) and the cost of the computation (low), but we should
consider thread safety here nonetheless.
At the very least, we should make `this.requestAttributes` volatile. Then
here we should construct a local map and add the values onto it, then only do
`this.requestAttributes = localAttributes` at the end. This way if there did
happen to be 2 callers, one would not get an incomplete view of the request
attributes.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcCall.java:
##########
@@ -83,7 +86,17 @@ public interface RpcCall extends RpcCallContext {
/** Returns The request header of this call. */
RequestHeader getHeader();
- ConnectionHeader getConnectionHeader();
+ /**
+ * Returns the map of attributes specified when building the Connection See
the Map argument on
Review Comment:
nitpick: since you already have to do one more revision can you fix this and
below javadoc to at least add a period prior to `See the..`? Could also add a
`<br>` if you want them to be on the next line, or better yet use `@see`, i.e.:
```java
/**
* Returns the map of attributes specified when building the Connection.
* @see ConnectionFactory#createConnection(Configuration, ExecutorService,
User, Map)
*/
```
The see will automatically also do the linking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
