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 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