-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API #2261
Conversation
virajjasani
commented
Aug 15, 2020
•
edited
Loading
edited
- Admin API getLogEntries() for ring buffer use-cases: so far, provides balancerDecision and slowLogResponse
- Refactor RPC call for similar use-cases
- Single RPC API getLogEntries() for both Master.proto and Admin.proto
…eve entries from HMaster queue
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of this is boilerplate and mechanism, which I largely don't have concerns about, but I do have a concern with lack of generalization in some places. I think we want to take steps now to reduce future work especially API modification/deprecation pains. What do you think?
* @return list of balancer decision records | ||
* @throws IOException if a remote or network exception occurs | ||
*/ | ||
List<BalancerDecisionRecords> getBalancerDecisions() throws IOException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just call this BalancerDecision ? Of course it's a record, or an item, or an element (choose your term for individual item in a collection) so that just adds letters for no clearer meaning.
private static final Gson GSON = GsonUtil.createGson() | ||
.setPrettyPrinting() | ||
.registerTypeAdapter(OnlineLogRecord.class, (JsonSerializer<OnlineLogRecord>) | ||
(slowLogPayload, type, jsonSerializationContext) -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is being used for more than just the slowlog, this parameter slowLogPayload should be renamed. (First question that comes to mind is what does slow log have to do with the balancer). Call it logPayload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't believe this miss! This Gson serialization function was a copy paste :|
@@ -1057,4 +1057,9 @@ public void updateRSGroupConfig(String groupName, Map<String, String> configurat | |||
throws IOException { | |||
get(admin.updateRSGroupConfig(groupName, configuration)); | |||
} | |||
|
|||
@Override | |||
public List<BalancerDecisionRecords> getBalancerDecisions() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something to consider: Rather than adding new API for every ringbuffer backed type, since the ringbuffers are named, can we just have one API that retrieves records from a buffer specified by name?
E.g.
public List<LogEntry> getLogEntries(String name)
Then, LogEntry is a generic type capable of accepting any protobuf encoding. Then, we derive new types from LogEntry such as BalancerDecision. Have a static method in LogEntry for instantiating the subclasses by reflection based on what type is communicated by the protobuf.
If LogEntry is too generic a name, consider RingEntry. (I'm not the best at naming, maybe someone else has a better idea...)
It is a lot easier to add or remove specialized classes as these things evolve than add or remove methods from public/stable admin APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would mean you'd have to go back and modify any admin API related to the slow log, which is fine, and desirable (if you accept the premise of this feedback)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have this type of API, it should be possible to provide a limit, e.g.
public List<LogEntry> getLogEntries(String name, int limit);
so that a client that is memory constrained (or wants to be frugal) doesn't have to worry about invoking this and maybe getting back a list of 5000 entries or whatever, by passing in a limit of 100, or 10, or ...
It would be fine to also provide a method that doesn't accept a limit, for convenience. Prerequisite: The methods are generic enough so we don't add a pair (or more!) for every type. E.g.
public List<LogEntry> getLogEntries(String name) { this(name, Integer.MAX_VALUE); }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely, adding new APIs not desirable, however few reasons why I had to choose this way:
- slowLog released in 2.3 already.
- slowLog is in each RS and the API has flexibility to choose serverName to get records from (filter by serverName) whereas balancer is HMaster queue meaning activeMasterStub is to be identified by client, hence no need of serverName filter.
- client has to identify whom to make a call - HMaster or RegionServer (or limited no of RS) based on use-case, not a big deal but maybe too much logic for client interface?
- genericAPI might need to include abstract request and response classes, and respective use-cases to ensure if filter is required at server side (not a big deal)
For 1), I believe trunk and 2.4.0 both can have generic API end-point.
2) and 3) seem bit of headache for single API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, that's the trade off here. Some pain now for flexibility later, or not much pain and no flexibility now or later.
New APIs for each new type is not a wrong choice per se. A benefit to API per ring type is pressure from API compat concerns and method proliferation will put a lot of pressure on new potential use cases for this mechanism, will set a really high bar. Maybe too high, hence my concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just to be clear, please take some time to think about this, but it is not a request for change per se.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but if you dont' accept the majority of this feedback, which would be fine, we at least need the limit parameter. And also add one for the slow log if there isn't one there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree method proliferation is a concern, something to consider and once we come to a decision, there is no way back it seems.
For limit param, we have limit in slowLog use-case because client is calling every single RS. Here, since it's just HMaster, the real limit should be kept in HMaster's ring buffer size. However, memory constraints for client makes limiting useful even for single server.
* History of balancer decisions taken for region movements. | ||
*/ | ||
@InterfaceAudience.Private | ||
final public class BalancerDecisionRecords { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just BalancerDecision? 'Records' doesn't add any meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
@@ -1277,4 +1278,9 @@ public void updateRSGroupConfig(String groupName, Map<String, String> configurat | |||
throws IOException { | |||
throw new NotImplementedException("updateRSGroupConfig not supported in ThriftAdmin"); | |||
} | |||
|
|||
@Override | |||
public List<BalancerDecisionRecords> getBalancerDecisions() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we had one admin API for master and regionservers that returned protobuf encoded entries, then the thrift API could support it. First cut could just pass through the encoded PB.
balancer_decisions_resp_arr << balancer_dec_resp.toJsonPrettyPrint | ||
} | ||
puts 'Retrieved BalancerDecision Responses from HMaster' | ||
puts balancer_decisions_resp_arr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only return the array here, this may be used for programmatic things. Put the part that prints the result into the command impl which invokes this admin.rb function.
@@ -30,7 +30,8 @@ | |||
public class NamedQueuePayload { | |||
|
|||
public enum NamedQueueEvent { | |||
SLOW_LOG | |||
SLOW_LOG, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish I had caught this earlier.
If we have to use an Enum here, add a constructor that defines an ordinal for each type so we can maintain compatibility by instantiating by our ordinal, e.g.
NamedQueueEvent(int ordinal) { ... }
SLOW_LOG(1),
BALANCE_DECISION(2),.
...
NamedQueueEvent getByOrdinal(int ordinal)
I believe we should have generic Admin API for similar use-cases. For existing slowLog use-case, I can make it default implementation pointing to generic API and provide deprecation notice (with removal in 4.0.0 release).
|
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@apurtell |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, some changes requested for consideration, and some nits
@@ -64,6 +66,7 @@ | |||
import org.apache.hadoop.hbase.snapshot.UnknownSnapshotException; | |||
import org.apache.hadoop.hbase.util.Bytes; | |||
import org.apache.hadoop.hbase.util.Pair; | |||
import org.apache.hbase.thirdparty.org.apache.commons.collections4.CollectionUtils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a nonshaded collections util available? Might be an issue when backporting. Just a nit.
* @return Log entries representing online records from servers | ||
* @throws IOException if a remote or network exception occurs | ||
*/ | ||
List<LogEntry> getLogEntries(LogRequest logRequest) throws IOException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs an optional parameter to allow constrained clients to limit the size of the returned list of LogEntry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can consider limit
as also one of the request attribute right? Hence, we are leaving upto individual use-cases to provide limit and both slowLog and balancerDecision do have limit
provided as part of their request payload and both use-cases consider limit:
(All references are for 1. BalancerDecision followed by 2. SlowLog)
1.
public class BalancerDecisionRequest extends LogRequest {
private int limit = 250;
public int getLimit() {
...
...
2.
public class LogQueryFilter extends LogRequest {
...
...
private int limit = 10;
...
...
And, here on the server side, we consider limiting records for both use-cases:
1.
public class BalancerDecisionQueueService implements NamedQueueService {
...
...
@Override
public NamedQueueGetResponse getNamedQueueRecords(NamedQueueGetRequest request) {
...
...
int limit = Math.min(request.getBalancerDecisionRequest().getLimit(), balancerDecisions.size());
// filter limit if provided
balancerDecisions = balancerDecisions.subList(0, limit);
...
...
2.
public class LogHandlerUtils {
...
...
public static List<TooSlowLog.SlowLogPayload> getFilteredLogs(
AdminProtos.SlowLogResponseRequest request, List<TooSlowLog.SlowLogPayload> logPayloadList) {
...
...
int limit = Math.min(request.getLimit(), logPayloadList.size());
return logPayloadList.subList(0, limit);
}
Also, this is where we convert Admin request payload to Protobuf request payload before passing on to servers over network call:
1.
private CompletableFuture<List<LogEntry>> getBalancerDecisions(
BalancerDecisionRequest balancerDecisionRequest) {
return this.<List<LogEntry>>newMasterCaller()
.action((controller, stub) ->
this.call(controller, stub,
MasterProtos.BalancerDecisionRequest.newBuilder()
/** => **/ .setLimit(balancerDecisionRequest.getLimit()).build(),
MasterService.Interface::getBalancerDecisions, ProtobufUtil::toBalancerDecisionResponse))
.call();
}
2.
public final class RequestConverter {
...
...
public static SlowLogResponseRequest buildSlowLogResponseRequest(
final LogQueryFilter logQueryFilter) {
...
...
return builder.setLimit(logQueryFilter.getLimit()).build();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please define a limit parameter for the Admin API. I'm open to other suggestions, but by "individual use cases" I believe you mean the RPC message classes, and those are not the user facing API, they are an implementation detail.
As an alternative you can do the same thing as you did for the SlowLog API where a filter can be supplied, and one thing the filter interface lets you do is specify a limit.
I won't approve this without a limit option of some kind in the public user facing admin API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is our Admin endpoint:
List<LogEntry> getLogEntries(LogRequest logRequest) throws IOException;
Now when client uses this to get slowLog response, they can provide limit
in specific payload which extends LogRequest
and send it over, similarly they can provide limit
in BalancerDecisionRequest which extends LogRequest
and get the response accordingly, both request payloads that extend LogRequest
are anyways public interfaces for clients. Now, client would rather provide limit
in request payload than as separate argument on this Admin endpoint (method signature). Does that sound good?
@@ -1057,4 +1051,9 @@ public void updateRSGroupConfig(String groupName, Map<String, String> configurat | |||
throws IOException { | |||
get(admin.updateRSGroupConfig(groupName, configuration)); | |||
} | |||
|
|||
@Override | |||
public List<LogEntry> getLogEntries(LogRequest logRequest) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment on admin interface has implications here. (This needs an optional parameter to allow constrained clients to limit the size of the returned list of LogEntry.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, limit
is included in request payload by individual use-cases.
* @param logRequest request payload with possible filters | ||
* @return Log entries representing online records from servers | ||
*/ | ||
CompletableFuture<List<LogEntry>> getLogEntries(LogRequest logRequest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment on admin interface has implications here. (This needs an optional parameter to allow constrained clients to limit the size of the returned list of LogEntry.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, limit
is included in request payload by individual use-cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please define a limit parameter for the Admin API. I'm open to other suggestions, but by "individual use cases" I believe you mean the RPC message classes, and those are not user facing public API. The user facing API here is the Admin API. This is the place to do this.
As an alternative you can do the same thing as you did for the SlowLog API where a filter can be supplied, and one thing the filter interface lets you do is specify a limit.
I won't approve this without a limit option of some kind in the public user facing admin API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future please do not mark conversations that are not resolved as resolved.
@@ -40,7 +40,7 @@ | |||
|
|||
public RpcLogDetails(RpcCall rpcCall, Message param, String clientAddress, long responseSize, | |||
String className, boolean isSlowLog, boolean isLargeLog) { | |||
super(NamedQueueEvent.SLOW_LOG); | |||
super(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea of using ordinals is we can encode them into message types and be more gentle at runtime if missing one (as opposed to what Java would do by default, a linkage error I think). However for code I think enums provide documentation of intent and magic constants should be avoided. Or, at least, use a static integer constant named "SLOW_LOG"...
@@ -201,7 +201,7 @@ public NamedQueueGetResponse getNamedQueueRecords(NamedQueueGetRequest request) | |||
slowLogPayloads = getSlowLogPayloads(slowLogResponseRequest); | |||
} | |||
NamedQueueGetResponse response = new NamedQueueGetResponse(); | |||
response.setNamedQueueEvent(NamedQueuePayload.NamedQueueEvent.SLOW_LOG); | |||
response.setNamedQueueEvent(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See related comment.
} | ||
|
||
@Override | ||
public String toString() { | ||
return new ToStringBuilder(this) | ||
.append("slowLogResponseRequest", slowLogResponseRequest) | ||
.append("namedQueueEvent", namedQueueEvent) | ||
.append("balancerDecisionRequest", balancerDecisionRequest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"balancerDecisionsRequest"
@@ -3966,7 +3966,7 @@ public SlowLogResponses getSlowLogResponses(final RpcController controller, | |||
} | |||
List<SlowLogPayload> slowLogPayloads; | |||
NamedQueueGetRequest namedQueueGetRequest = new NamedQueueGetRequest(); | |||
namedQueueGetRequest.setNamedQueueEvent(NamedQueuePayload.NamedQueueEvent.SLOW_LOG); | |||
namedQueueGetRequest.setNamedQueueEvent(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See related comment.
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncTableGetMultiThreaded.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review @apurtell
Reg limit
param, we can treat it as one of the request payload params for Admin API and hence, both slowLog
and balancerDecision
Admin request params contain limit
.
|
||
private int limit = 250; | ||
|
||
public int getLimit() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the request param coming to Admin API.
Detailed comment on Admin API: limit
is included in request payload by individual use-cases.
* @return Log entries representing online records from servers | ||
* @throws IOException if a remote or network exception occurs | ||
*/ | ||
List<LogEntry> getLogEntries(LogRequest logRequest) throws IOException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can consider limit
as also one of the request attribute right? Hence, we are leaving upto individual use-cases to provide limit and both slowLog and balancerDecision do have limit
provided as part of their request payload and both use-cases consider limit:
(All references are for 1. BalancerDecision followed by 2. SlowLog)
1.
public class BalancerDecisionRequest extends LogRequest {
private int limit = 250;
public int getLimit() {
...
...
2.
public class LogQueryFilter extends LogRequest {
...
...
private int limit = 10;
...
...
And, here on the server side, we consider limiting records for both use-cases:
1.
public class BalancerDecisionQueueService implements NamedQueueService {
...
...
@Override
public NamedQueueGetResponse getNamedQueueRecords(NamedQueueGetRequest request) {
...
...
int limit = Math.min(request.getBalancerDecisionRequest().getLimit(), balancerDecisions.size());
// filter limit if provided
balancerDecisions = balancerDecisions.subList(0, limit);
...
...
2.
public class LogHandlerUtils {
...
...
public static List<TooSlowLog.SlowLogPayload> getFilteredLogs(
AdminProtos.SlowLogResponseRequest request, List<TooSlowLog.SlowLogPayload> logPayloadList) {
...
...
int limit = Math.min(request.getLimit(), logPayloadList.size());
return logPayloadList.subList(0, limit);
}
Also, this is where we convert Admin request payload to Protobuf request payload before passing on to servers over network call:
1.
private CompletableFuture<List<LogEntry>> getBalancerDecisions(
BalancerDecisionRequest balancerDecisionRequest) {
return this.<List<LogEntry>>newMasterCaller()
.action((controller, stub) ->
this.call(controller, stub,
MasterProtos.BalancerDecisionRequest.newBuilder()
/** => **/ .setLimit(balancerDecisionRequest.getLimit()).build(),
MasterService.Interface::getBalancerDecisions, ProtobufUtil::toBalancerDecisionResponse))
.call();
}
2.
public final class RequestConverter {
...
...
public static SlowLogResponseRequest buildSlowLogResponseRequest(
final LogQueryFilter logQueryFilter) {
...
...
return builder.setLimit(logQueryFilter.getLimit()).build();
}
@@ -1057,4 +1051,9 @@ public void updateRSGroupConfig(String groupName, Map<String, String> configurat | |||
throws IOException { | |||
get(admin.updateRSGroupConfig(groupName, configuration)); | |||
} | |||
|
|||
@Override | |||
public List<LogEntry> getLogEntries(LogRequest logRequest) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, limit
is included in request payload by individual use-cases.
* @param logRequest request payload with possible filters | ||
* @return Log entries representing online records from servers | ||
*/ | ||
CompletableFuture<List<LogEntry>> getLogEntries(LogRequest logRequest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, limit
is included in request payload by individual use-cases.
@@ -693,6 +694,14 @@ message SwitchExceedThrottleQuotaResponse { | |||
required bool previous_exceed_throttle_quota_enabled = 1; | |||
} | |||
|
|||
message BalancerDecisionRequest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, this is just request payload we are sending over to RPC endpoint. But yeah, it could be Decisions
@@ -222,6 +226,14 @@ public synchronized void setConf(Configuration conf) { | |||
|
|||
curFunctionCosts= new Double[costFunctions.size()]; | |||
tempFunctionCosts= new Double[costFunctions.size()]; | |||
|
|||
boolean isBalancerDecisionEnabled = getConf() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely, with verb it looks good.
hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some cleanup needed for stuff left over from earlier revisions, then I will approve.
* Retrieve recent online records from HMaster / RegionServers. | ||
* Examples include slow/large RPC logs, balancer decisions by master. | ||
* | ||
* @param serverNames servers to retrieve records from, useful in case of records maintained by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when we have multiple masters? I think just a doc update is needed here to indicate the log for servertype=MASTER will only come from the currently active master. Can be done at commit time.
*/ | ||
@InterfaceAudience.Public | ||
public enum ServerType { | ||
HMASTER, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try to not use 'H' prefixes in new code. Please just call these MASTER and REGION_SERVER. Can be fixed at commit time.
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
Show resolved
Hide resolved
...c/main/java/org/apache/hadoop/hbase/master/balancer/MetricsStochasticBalancerSourceImpl.java
Show resolved
Hide resolved
message LogRequest { | ||
required string log_class_name = 1; | ||
required bytes log_initializer_message = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why "initializer"? What does that mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dang, it doesn't look good now. Context was different earlier. Let me keep it log_message
, it's payload now.
optional uint32 limit = 1; | ||
} | ||
|
||
message BalancerDecisionsResponse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need these?
We have generic LogRequest now, and the log request gives the name of the log we want to return (encapsulated) results from.
Protobuf for BalancerDecisions is fine!
But the BalancerDecisionsRequest and BalancerDecisionsResponse proto types are for an RPC interface from an earlier revision of this patch, and that (specialized) RPC no longer exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
- Define request and response message. Which will be encoded in bytes and sent to generic RPC API
getLogEntries
(BalancerDecisionsRequest and BalancerDecisionsResponse in this case) - Define message for use-case specific payload that we want to return to end user. (message BalancerDecision in this case)
- 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
Please add a comment that explains this in the proto file where these definitions are made. Can be done at commit time, np.
hbase-server/src/main/java/org/apache/hadoop/hbase/namequeues/NamedQueuePayload.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressing concerns, an addendum commit will follow.
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
Show resolved
Hide resolved
...c/main/java/org/apache/hadoop/hbase/master/balancer/MetricsStochasticBalancerSourceImpl.java
Show resolved
Hide resolved
message LogRequest { | ||
required string log_class_name = 1; | ||
required bytes log_initializer_message = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dang, it doesn't look good now. Context was different earlier. Let me keep it log_message
, it's payload now.
optional uint32 limit = 1; | ||
} | ||
|
||
message BalancerDecisionsResponse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
- Define request and response message. Which will be encoded in bytes and sent to generic RPC API
getLogEntries
(BalancerDecisionsRequest and BalancerDecisionsResponse in this case) - Define message for use-case specific payload that we want to return to end user. (message BalancerDecision in this case)
- 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.
hbase-server/src/main/java/org/apache/hadoop/hbase/namequeues/NamedQueuePayload.java
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ship it!
optional uint32 limit = 1; | ||
} | ||
|
||
message BalancerDecisionsResponse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
Please add a comment that explains this in the proto file where these definitions are made. Can be done at commit time, np.
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
…dmin API * Admin API getLogEntries() for ring buffer use-cases: so far, provides balancerDecision and slowLogResponse * Refactor RPC call for similar use-cases * Single RPC API getLogEntries() for both Master.proto and Admin.proto Closes apache#2261 Signed-off-by: Andrew Purtell <apurtell@apache.org>
…dmin API (#2411) * Admin API getLogEntries() for ring buffer use-cases: so far, provides balancerDecision and slowLogResponse * Refactor RPC call for similar use-cases * Single RPC API getLogEntries() for both Master.proto and Admin.proto Closes #2261 Signed-off-by: Andrew Purtell <apurtell@apache.org>
…dmin API (apache#2411) * Admin API getLogEntries() for ring buffer use-cases: so far, provides balancerDecision and slowLogResponse * Refactor RPC call for similar use-cases * Single RPC API getLogEntries() for both Master.proto and Admin.proto Closes apache#2261 Signed-off-by: Andrew Purtell <apurtell@apache.org>