virajjasani commented on a change in pull request #754: HBASE-22978 : Online slow response log URL: https://github.com/apache/hbase/pull/754#discussion_r379974849
########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/slowlog/SlowLogEventHandler.java ########## @@ -0,0 +1,154 @@ +/* + * + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hbase.regionserver.slowlog; + +import com.lmax.disruptor.EventHandler; +import com.lmax.disruptor.RingBuffer; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Queue; +import java.util.stream.Collectors; + +import org.apache.commons.lang3.StringUtils; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.apache.hbase.thirdparty.com.google.common.collect.EvictingQueue; +import org.apache.hbase.thirdparty.com.google.common.collect.Queues; + +import org.apache.hadoop.hbase.shaded.protobuf.generated.AdminProtos; +import org.apache.hadoop.hbase.shaded.protobuf.generated.TooSlowLog.SlowLogPayload; + +/** + * Event Handler run by disruptor ringbuffer consumer + */ +@InterfaceAudience.Private +class SlowLogEventHandler implements EventHandler<RingBufferEnvelope> { + + private static final Logger LOG = LoggerFactory.getLogger(SlowLogEventHandler.class); + + private final Queue<SlowLogPayload> queue; + + SlowLogEventHandler(int eventCount) { + EvictingQueue<SlowLogPayload> evictingQueue = EvictingQueue.create(eventCount); Review comment: I am not in favor of removing Lmax Disruptor completely, in fact when it comes to ring buffer in Java, Disruptor provides the best performance due to mechanical sympathy. As of now, our usecase might not be complex enough to have significant perf improvement using Disruptor but this is supposed to get more data e.g. tooLong records too in ring buffer, more usecases - keeping too long and too slow together etc. Thinking of all that, we should definitely have Disruptor here. At consumer side, EvictingQueue makes sense because as I mentioned, in future, for different usecases, we can have different DataStructures e.g. separate EvictingQueue for tooSlow, tooLarge etc and additional logic if required for filter purpose etc. All these consumer logic can change based on requirement specifically from Get API POV, how we want to retrieve them. But if we remove the Disruptor, we will have to start dealing with consumer specific logic in producer i.e. send tooSlow event to one specific queue, send tooLarge or some other event to other queue etc which doesn't seem right. Hence, I am not onboard with removal of Disruptor completely which is the primary entry point for all logs of our interest. ---------------------------------------------------------------- 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
