Github user jyates commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/55#discussion_r27270440
--- Diff:
phoenix-core/src/main/java/org/apache/hadoop/hbase/ipc/PhoenixRpcScheduler.java
---
@@ -40,28 +40,34 @@
private static final int DEFAULT_MAX_CALLQUEUE_LENGTH_PER_HANDLER = 10;
private RpcScheduler delegate;
- private int minPriority;
- private int maxPriority;
- private RpcExecutor callExecutor;
+ private int indexPriority;
+ private int metadataPriority;
+ private RpcExecutor indexCallExecutor;
+ private RpcExecutor metadataCallExecutor;
private int port;
- public PhoenixIndexRpcScheduler(int indexHandlerCount, Configuration
conf,
- RpcScheduler delegate, int minPriority, int maxPriority) {
+ public PhoenixRpcScheduler(int indexHandlerCount, int
metadataHandlerCount, Configuration conf,
--- End diff --
> DelegatingPayloadCarryingRpcController.
Huh? That's an odd one to pick.
It seemed to me that you wanted to do something that looked like an
RpcScheduler (maybe drop all the priority queue, replication queue methods) and
change dispatch to look like ```boolean dispatch(CallRunner callTask)``` where
boolean indicates handled or not.
> needs to implement getGeneralQueueLength and getActiveRpcHandlerCount
If you do the 'kinda like an RpcScheduler' route then you just loop through
all of them and sum the counts. They don't need to be instantaneously accurate
as its just being used for metrics, not for anything critical and need to 100%
taken at the same point (really impossible if you have more than 1 queue).
> would be easier to have a map from prioirity to queue and then in
dispatch() we just lookup the queue based on the priority
Not necessarily, as priority isn't the only thing that determines which
queue an RPC should be sent to. For instance, QosPriority annotations can be
used to determine which queue an RPC should be sent to as well:
```
@QosPriority(priority=HConstants.HIGH_QOS)
public GetRegionInfoResponse getRegionInfo(final RpcController controller,
final GetRegionInfoRequest request) throws ServiceException {
```
Now that you have implemented something like what I recommended, do you
think its cleaner? easier to reason about? I was recommending based on what
seemed like an anti-pattern in favor of a pretty reasonable abstraction from
unix kernel hooks (and adopted by HBase for coprocessors)...however, I'm known
to be a little heavy on generalizations, so this could be overkill to. What do
you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
