virajjasani commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r487408079
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##########
@@ -3626,4 +3643,49 @@ public static CheckAndMutate
toCheckAndMutate(ClientProtos.Condition condition,
throw new DoNotRetryIOException(e.getMessage());
}
}
+
+ public static List<LogEntry> toBalancerDecisionResponse(
+ HBaseProtos.LogEntry logEntry) {
+ try {
+ final String logClassName = logEntry.getLogClassName();
+ Class<?> logClass =
Class.forName(logClassName).asSubclass(Message.class);
+ Method method = logClass.getMethod("parseFrom", ByteString.class);
+ if (logClassName.contains("BalancerDecisionsResponse")) {
Review comment:
Same as above, last line of method after try/catch:
```
throw new RuntimeException("Invalid response from server");
```
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/namequeues/NamedQueuePayload.java
##########
@@ -30,16 +30,39 @@
public class NamedQueuePayload {
public enum NamedQueueEvent {
- SLOW_LOG
+ SLOW_LOG(0),
Review comment:
I understand your concern but this is anyways server side code and at
server side, we need to add new ring buffer implementor that implements
`NamedQueueService`.
And one of the methods need to be implemented is:
```
/**
* Retrieve event type for NamedQueueService implementation.
*
* @return {@link NamedQueuePayload.NamedQueueEvent}
*/
NamedQueuePayload.NamedQueueEvent getEvent();
```
Which is used by main LogEventHandler to maintain map of Event ->
Implementor logic. I believe creating new enum entry with ordinal should be
good enough for server side implementation.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##########
@@ -3503,14 +3508,26 @@ private static OnlineLogRecord getSlowLogRecord(
/**
* Convert AdminProtos#SlowLogResponses to list of {@link OnlineLogRecord}
*
- * @param slowLogResponses slowlog response protobuf instance
+ * @param logEntry slowlog response protobuf instance
* @return list of SlowLog payloads for client usecase
*/
- public static List<OnlineLogRecord> toSlowLogPayloads(
- final AdminProtos.SlowLogResponses slowLogResponses) {
- List<OnlineLogRecord> onlineLogRecords =
slowLogResponses.getSlowLogPayloadsList()
-
.stream().map(ProtobufUtil::getSlowLogRecord).collect(Collectors.toList());
- return onlineLogRecords;
+ public static List<LogEntry> toSlowLogPayloads(
+ final HBaseProtos.LogEntry logEntry) {
+ try {
+ final String logClassName = logEntry.getLogClassName();
+ Class<?> logClass =
Class.forName(logClassName).asSubclass(Message.class);
+ Method method = logClass.getMethod("parseFrom", ByteString.class);
+ if (logClassName.contains("SlowLogResponses")) {
Review comment:
Oh, we don't leave it up to client, we do throw Exception. The last line
of this method after try/catch is over:
```
throw new RuntimeException("Invalid response from server");
```
I realize since it's last line of the method, while reading, it's bit
difficult to catch.
Any specific Exception message recommendation? The reason why I kept
"Invalid response from server" is because ultimately we are parsing response
from RPC call.
Open to update error message for better recommendation.
##########
File path: hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto
##########
@@ -693,6 +694,14 @@ message SwitchExceedThrottleQuotaResponse {
required bool previous_exceed_throttle_quota_enabled = 1;
}
+message BalancerDecisionsRequest {
+ optional uint32 limit = 1;
+}
+
+message BalancerDecisionsResponse {
Review comment:
Although specialized RPC does not exist anymore, we would want client to
encode use-case specific request (BalancerDecisionsRequest) in bytes to generic
RPC and also retrieve response and then decode bytes into use-case specific
response (BalancerDecisionsResponse, which wraps `BalancerDecision` proto).
Again, this decision comes for relatively easy-to-understand code. What do
we need to do for a new ring buffer use-case?
1. Define request and response message. Which will be encoded in bytes and
sent to generic RPC API `getLogEntries` (BalancerDecisionsRequest and
BalancerDecisionsResponse in this case)
2. Define message for use-case specific payload that we want to return to
end user. (message BalancerDecision in this case)
3. Add parsing logic in ProtobufUtil.
For our use-cases:
```
if (logClassName.contains("SlowLogResponses")) {
```
and
```
if (logClassName.contains("BalancerDecisionsResponse")) {
```
We don't need a new RPC or Admin API, but good to have new request/response
message which can be encoded within generic `LogRequest` and `LogResponse` and
the relevant parsing logic becomes easy to grasp.
##########
File path: hbase-protocol-shaded/src/main/protobuf/HBase.proto
##########
@@ -273,4 +273,14 @@ message RegionLocation {
required RegionInfo region_info = 1;
optional ServerName server_name = 2;
required int64 seq_num = 3;
-}
\ No newline at end of file
+}
+
+message LogRequest {
+ required string log_class_name = 1;
+ required bytes log_initializer_message = 2;
Review comment:
Dang, it doesn't look good now. Context was different earlier. Let me
keep it `log_message`, it's payload now.
##########
File path:
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/master/balancer/MetricsStochasticBalancerSourceImpl.java
##########
@@ -37,7 +37,7 @@
private int metricsSize = 1000;
private int mruCap = calcMruCap(metricsSize);
- private Map<String, Map<String, Double>> stochasticCosts =
+ private final Map<String, Map<String, Double>> stochasticCosts =
Review comment:
Yeah trivial improvement. Let's keep it if you are fine?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##########
@@ -3626,4 +3643,49 @@ public static CheckAndMutate
toCheckAndMutate(ClientProtos.Condition condition,
throw new DoNotRetryIOException(e.getMessage());
}
}
+
+ public static List<LogEntry> toBalancerDecisionResponse(
+ HBaseProtos.LogEntry logEntry) {
+ try {
+ final String logClassName = logEntry.getLogClassName();
+ Class<?> logClass =
Class.forName(logClassName).asSubclass(Message.class);
+ Method method = logClass.getMethod("parseFrom", ByteString.class);
+ if (logClassName.contains("BalancerDecisionsResponse")) {
Review comment:
Same as above, last line of method after try/catch:
```
throw new RuntimeException("Invalid response from server");
```
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/namequeues/NamedQueuePayload.java
##########
@@ -30,16 +30,39 @@
public class NamedQueuePayload {
public enum NamedQueueEvent {
- SLOW_LOG
+ SLOW_LOG(0),
Review comment:
I understand your concern but this is anyways server side code and at
server side, we need to add new ring buffer implementor that implements
`NamedQueueService`.
And one of the methods need to be implemented is:
```
/**
* Retrieve event type for NamedQueueService implementation.
*
* @return {@link NamedQueuePayload.NamedQueueEvent}
*/
NamedQueuePayload.NamedQueueEvent getEvent();
```
Which is used by main LogEventHandler to maintain map of Event ->
Implementor logic. I believe creating new enum entry with ordinal should be
good enough for server side implementation.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##########
@@ -3503,14 +3508,26 @@ private static OnlineLogRecord getSlowLogRecord(
/**
* Convert AdminProtos#SlowLogResponses to list of {@link OnlineLogRecord}
*
- * @param slowLogResponses slowlog response protobuf instance
+ * @param logEntry slowlog response protobuf instance
* @return list of SlowLog payloads for client usecase
*/
- public static List<OnlineLogRecord> toSlowLogPayloads(
- final AdminProtos.SlowLogResponses slowLogResponses) {
- List<OnlineLogRecord> onlineLogRecords =
slowLogResponses.getSlowLogPayloadsList()
-
.stream().map(ProtobufUtil::getSlowLogRecord).collect(Collectors.toList());
- return onlineLogRecords;
+ public static List<LogEntry> toSlowLogPayloads(
+ final HBaseProtos.LogEntry logEntry) {
+ try {
+ final String logClassName = logEntry.getLogClassName();
+ Class<?> logClass =
Class.forName(logClassName).asSubclass(Message.class);
+ Method method = logClass.getMethod("parseFrom", ByteString.class);
+ if (logClassName.contains("SlowLogResponses")) {
Review comment:
Oh, we don't leave it up to client, we do throw Exception. The last line
of this method after try/catch is over:
```
throw new RuntimeException("Invalid response from server");
```
I realize since it's last line of the method, while reading, it's bit
difficult to catch.
Any specific Exception message recommendation? The reason why I kept
"Invalid response from server" is because ultimately we are parsing response
from RPC call.
Open to update error message for better recommendation.
##########
File path: hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto
##########
@@ -693,6 +694,14 @@ message SwitchExceedThrottleQuotaResponse {
required bool previous_exceed_throttle_quota_enabled = 1;
}
+message BalancerDecisionsRequest {
+ optional uint32 limit = 1;
+}
+
+message BalancerDecisionsResponse {
Review comment:
Although specialized RPC does not exist anymore, we would want client to
encode use-case specific request (BalancerDecisionsRequest) in bytes to generic
RPC and also retrieve response and then decode bytes into use-case specific
response (BalancerDecisionsResponse, which wraps `BalancerDecision` proto).
Again, this decision comes for relatively easy-to-understand code. What do
we need to do for a new ring buffer use-case?
1. Define request and response message. Which will be encoded in bytes and
sent to generic RPC API `getLogEntries` (BalancerDecisionsRequest and
BalancerDecisionsResponse in this case)
2. Define message for use-case specific payload that we want to return to
end user. (message BalancerDecision in this case)
3. Add parsing logic in ProtobufUtil.
For our use-cases:
```
if (logClassName.contains("SlowLogResponses")) {
```
and
```
if (logClassName.contains("BalancerDecisionsResponse")) {
```
We don't need a new RPC or Admin API, but good to have new request/response
message which can be encoded within generic `LogRequest` and `LogResponse` and
the relevant parsing logic becomes easy to grasp.
##########
File path: hbase-protocol-shaded/src/main/protobuf/HBase.proto
##########
@@ -273,4 +273,14 @@ message RegionLocation {
required RegionInfo region_info = 1;
optional ServerName server_name = 2;
required int64 seq_num = 3;
-}
\ No newline at end of file
+}
+
+message LogRequest {
+ required string log_class_name = 1;
+ required bytes log_initializer_message = 2;
Review comment:
Dang, it doesn't look good now. Context was different earlier. Let me
keep it `log_message`, it's payload now.
##########
File path:
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/master/balancer/MetricsStochasticBalancerSourceImpl.java
##########
@@ -37,7 +37,7 @@
private int metricsSize = 1000;
private int mruCap = calcMruCap(metricsSize);
- private Map<String, Map<String, Double>> stochasticCosts =
+ private final Map<String, Map<String, Double>> stochasticCosts =
Review comment:
Yeah trivial improvement. Let's keep it if you are fine?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##########
@@ -3626,4 +3643,49 @@ public static CheckAndMutate
toCheckAndMutate(ClientProtos.Condition condition,
throw new DoNotRetryIOException(e.getMessage());
}
}
+
+ public static List<LogEntry> toBalancerDecisionResponse(
+ HBaseProtos.LogEntry logEntry) {
+ try {
+ final String logClassName = logEntry.getLogClassName();
+ Class<?> logClass =
Class.forName(logClassName).asSubclass(Message.class);
+ Method method = logClass.getMethod("parseFrom", ByteString.class);
+ if (logClassName.contains("BalancerDecisionsResponse")) {
Review comment:
Same as above, last line of method after try/catch:
```
throw new RuntimeException("Invalid response from server");
```
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/namequeues/NamedQueuePayload.java
##########
@@ -30,16 +30,39 @@
public class NamedQueuePayload {
public enum NamedQueueEvent {
- SLOW_LOG
+ SLOW_LOG(0),
Review comment:
I understand your concern but this is anyways server side code and at
server side, we need to add new ring buffer implementor that implements
`NamedQueueService`.
And one of the methods need to be implemented is:
```
/**
* Retrieve event type for NamedQueueService implementation.
*
* @return {@link NamedQueuePayload.NamedQueueEvent}
*/
NamedQueuePayload.NamedQueueEvent getEvent();
```
Which is used by main LogEventHandler to maintain map of Event ->
Implementor logic. I believe creating new enum entry with ordinal should be
good enough for server side implementation.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##########
@@ -3503,14 +3508,26 @@ private static OnlineLogRecord getSlowLogRecord(
/**
* Convert AdminProtos#SlowLogResponses to list of {@link OnlineLogRecord}
*
- * @param slowLogResponses slowlog response protobuf instance
+ * @param logEntry slowlog response protobuf instance
* @return list of SlowLog payloads for client usecase
*/
- public static List<OnlineLogRecord> toSlowLogPayloads(
- final AdminProtos.SlowLogResponses slowLogResponses) {
- List<OnlineLogRecord> onlineLogRecords =
slowLogResponses.getSlowLogPayloadsList()
-
.stream().map(ProtobufUtil::getSlowLogRecord).collect(Collectors.toList());
- return onlineLogRecords;
+ public static List<LogEntry> toSlowLogPayloads(
+ final HBaseProtos.LogEntry logEntry) {
+ try {
+ final String logClassName = logEntry.getLogClassName();
+ Class<?> logClass =
Class.forName(logClassName).asSubclass(Message.class);
+ Method method = logClass.getMethod("parseFrom", ByteString.class);
+ if (logClassName.contains("SlowLogResponses")) {
Review comment:
Oh, we don't leave it up to client, we do throw Exception. The last line
of this method after try/catch is over:
```
throw new RuntimeException("Invalid response from server");
```
I realize since it's last line of the method, while reading, it's bit
difficult to catch.
Any specific Exception message recommendation? The reason why I kept
"Invalid response from server" is because ultimately we are parsing response
from RPC call.
Open to update error message for better recommendation.
##########
File path: hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto
##########
@@ -693,6 +694,14 @@ message SwitchExceedThrottleQuotaResponse {
required bool previous_exceed_throttle_quota_enabled = 1;
}
+message BalancerDecisionsRequest {
+ optional uint32 limit = 1;
+}
+
+message BalancerDecisionsResponse {
Review comment:
Although specialized RPC does not exist anymore, we would want client to
encode use-case specific request (BalancerDecisionsRequest) in bytes to generic
RPC and also retrieve response and then decode bytes into use-case specific
response (BalancerDecisionsResponse, which wraps `BalancerDecision` proto).
Again, this decision comes for relatively easy-to-understand code. What do
we need to do for a new ring buffer use-case?
1. Define request and response message. Which will be encoded in bytes and
sent to generic RPC API `getLogEntries` (BalancerDecisionsRequest and
BalancerDecisionsResponse in this case)
2. Define message for use-case specific payload that we want to return to
end user. (message BalancerDecision in this case)
3. Add parsing logic in ProtobufUtil.
For our use-cases:
```
if (logClassName.contains("SlowLogResponses")) {
```
and
```
if (logClassName.contains("BalancerDecisionsResponse")) {
```
We don't need a new RPC or Admin API, but good to have new request/response
message which can be encoded within generic `LogRequest` and `LogResponse` and
the relevant parsing logic becomes easy to grasp.
##########
File path: hbase-protocol-shaded/src/main/protobuf/HBase.proto
##########
@@ -273,4 +273,14 @@ message RegionLocation {
required RegionInfo region_info = 1;
optional ServerName server_name = 2;
required int64 seq_num = 3;
-}
\ No newline at end of file
+}
+
+message LogRequest {
+ required string log_class_name = 1;
+ required bytes log_initializer_message = 2;
Review comment:
Dang, it doesn't look good now. Context was different earlier. Let me
keep it `log_message`, it's payload now.
##########
File path:
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/master/balancer/MetricsStochasticBalancerSourceImpl.java
##########
@@ -37,7 +37,7 @@
private int metricsSize = 1000;
private int mruCap = calcMruCap(metricsSize);
- private Map<String, Map<String, Double>> stochasticCosts =
+ private final Map<String, Map<String, Double>> stochasticCosts =
Review comment:
Yeah trivial improvement. Let's keep it if you are fine?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##########
@@ -3626,4 +3643,49 @@ public static CheckAndMutate
toCheckAndMutate(ClientProtos.Condition condition,
throw new DoNotRetryIOException(e.getMessage());
}
}
+
+ public static List<LogEntry> toBalancerDecisionResponse(
+ HBaseProtos.LogEntry logEntry) {
+ try {
+ final String logClassName = logEntry.getLogClassName();
+ Class<?> logClass =
Class.forName(logClassName).asSubclass(Message.class);
+ Method method = logClass.getMethod("parseFrom", ByteString.class);
+ if (logClassName.contains("BalancerDecisionsResponse")) {
Review comment:
Same as above, last line of method after try/catch:
```
throw new RuntimeException("Invalid response from server");
```
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/namequeues/NamedQueuePayload.java
##########
@@ -30,16 +30,39 @@
public class NamedQueuePayload {
public enum NamedQueueEvent {
- SLOW_LOG
+ SLOW_LOG(0),
Review comment:
I understand your concern but this is anyways server side code and at
server side, we need to add new ring buffer implementor that implements
`NamedQueueService`.
And one of the methods need to be implemented is:
```
/**
* Retrieve event type for NamedQueueService implementation.
*
* @return {@link NamedQueuePayload.NamedQueueEvent}
*/
NamedQueuePayload.NamedQueueEvent getEvent();
```
Which is used by main LogEventHandler to maintain map of Event ->
Implementor logic. I believe creating new enum entry with ordinal should be
good enough for server side implementation.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##########
@@ -3503,14 +3508,26 @@ private static OnlineLogRecord getSlowLogRecord(
/**
* Convert AdminProtos#SlowLogResponses to list of {@link OnlineLogRecord}
*
- * @param slowLogResponses slowlog response protobuf instance
+ * @param logEntry slowlog response protobuf instance
* @return list of SlowLog payloads for client usecase
*/
- public static List<OnlineLogRecord> toSlowLogPayloads(
- final AdminProtos.SlowLogResponses slowLogResponses) {
- List<OnlineLogRecord> onlineLogRecords =
slowLogResponses.getSlowLogPayloadsList()
-
.stream().map(ProtobufUtil::getSlowLogRecord).collect(Collectors.toList());
- return onlineLogRecords;
+ public static List<LogEntry> toSlowLogPayloads(
+ final HBaseProtos.LogEntry logEntry) {
+ try {
+ final String logClassName = logEntry.getLogClassName();
+ Class<?> logClass =
Class.forName(logClassName).asSubclass(Message.class);
+ Method method = logClass.getMethod("parseFrom", ByteString.class);
+ if (logClassName.contains("SlowLogResponses")) {
Review comment:
Oh, we don't leave it up to client, we do throw Exception. The last line
of this method after try/catch is over:
```
throw new RuntimeException("Invalid response from server");
```
I realize since it's last line of the method, while reading, it's bit
difficult to catch.
Any specific Exception message recommendation? The reason why I kept
"Invalid response from server" is because ultimately we are parsing response
from RPC call.
Open to update error message for better recommendation.
##########
File path: hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto
##########
@@ -693,6 +694,14 @@ message SwitchExceedThrottleQuotaResponse {
required bool previous_exceed_throttle_quota_enabled = 1;
}
+message BalancerDecisionsRequest {
+ optional uint32 limit = 1;
+}
+
+message BalancerDecisionsResponse {
Review comment:
Although specialized RPC does not exist anymore, we would want client to
encode use-case specific request (BalancerDecisionsRequest) in bytes to generic
RPC and also retrieve response and then decode bytes into use-case specific
response (BalancerDecisionsResponse, which wraps `BalancerDecision` proto).
Again, this decision comes for relatively easy-to-understand code. What do
we need to do for a new ring buffer use-case?
1. Define request and response message. Which will be encoded in bytes and
sent to generic RPC API `getLogEntries` (BalancerDecisionsRequest and
BalancerDecisionsResponse in this case)
2. Define message for use-case specific payload that we want to return to
end user. (message BalancerDecision in this case)
3. Add parsing logic in ProtobufUtil.
For our use-cases:
```
if (logClassName.contains("SlowLogResponses")) {
```
and
```
if (logClassName.contains("BalancerDecisionsResponse")) {
```
We don't need a new RPC or Admin API, but good to have new request/response
message which can be encoded within generic `LogRequest` and `LogResponse` and
the relevant parsing logic becomes easy to grasp.
##########
File path: hbase-protocol-shaded/src/main/protobuf/HBase.proto
##########
@@ -273,4 +273,14 @@ message RegionLocation {
required RegionInfo region_info = 1;
optional ServerName server_name = 2;
required int64 seq_num = 3;
-}
\ No newline at end of file
+}
+
+message LogRequest {
+ required string log_class_name = 1;
+ required bytes log_initializer_message = 2;
Review comment:
Dang, it doesn't look good now. Context was different earlier. Let me
keep it `log_message`, it's payload now.
##########
File path:
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/master/balancer/MetricsStochasticBalancerSourceImpl.java
##########
@@ -37,7 +37,7 @@
private int metricsSize = 1000;
private int mruCap = calcMruCap(metricsSize);
- private Map<String, Map<String, Double>> stochasticCosts =
+ private final Map<String, Map<String, Double>> stochasticCosts =
Review comment:
Yeah trivial improvement. Let's keep it if you are fine?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
