virajjasani commented on a change in pull request #754: HBASE-22978 : Online
slow response log
URL: https://github.com/apache/hbase/pull/754#discussion_r371631514
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
##########
@@ -590,6 +596,10 @@ public HRegionServer(final Configuration conf) throws
IOException {
this.abortRequested = false;
this.stopped = false;
+ // initiate online slowlog ringbuffer only for RegionServers
+ if (!(this instanceof HMaster)) {
Review comment:
`protected void` would have helped if something was going to run as part of
`public void run()` of HRegionServer but here we are just worried about
initializing an object for HRegionServer and not for HMaster.
Honestly I don't like inheritance checking either but do we really have a
better way? Like do we have some internal config to check this or some param
may be?
e.g. I see lot of these codes:
```
if (server instanceof HRegionServer) {
rsServerHost = ((HRegionServer)
server).getRegionServerCoprocessorHost();
}
```
```
if (server instanceof HRegionServer) {
NettyEventLoopGroupConfig config = ((HRegionServer)
server).getEventLoopGroupConfig();
eventLoopGroup = config.group();
channelClass = config.serverChannelClass();
} else {
eventLoopGroup = new NioEventLoopGroup(0,
new DefaultThreadFactory("NettyRpcServer", true,
Thread.MAX_PRIORITY));
channelClass = NioServerSocketChannel.class;
}
```
```
boolean isMasterNotCarryTable =
this instanceof HMaster && !LoadBalancer.isTablesOnMaster(conf);
```
and so on. My point is we are trying to intialize something for RS and not
for HM and may be I am not aware of better decision making conditions that we
might already have. Could you please point me to one? Would be really helpful.
Here, a `protected void postConstruction` method not help right? Since we have
to initialize something in parent class and not in child class. We don't want
to make super() from child class initialize something without knowing the
instance and then let child re-initialize(override) it with null or something
similar?
I just saw this:
```
protected String getProcessName() {
return REGIONSERVER;
}
```
```
@Override
protected String getProcessName() {
return MASTER;
}
```
However, decision making based on this method is also not any better than
`instanceof` check
----------------------------------------------------------------
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
With regards,
Apache Git Services
