subject:"\[GitHub\] \[hbase\] anoopsjohn commented on a change in pull request #1681\: HBASE\-23938 \: System table hbase\:slowlog to store complete slow\/large…"

[GitHub] [hbase] anoopsjohn commented on a change in pull request #1681: HBASE-23938 : System table hbase:slowlog to store complete slow/large…

2020-05-19 Thread GitBox



anoopsjohn commented on a change in pull request #1681:
URL: https://github.com/apache/hbase/pull/1681#discussion_r427492504



##
File path: 
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcControllerImpl.java
##
@@ -101,8 +102,12 @@ public void setPriority(int priority) {
 
   @Override
   public void setPriority(final TableName tn) {
-setPriority(
-  tn != null && tn.isSystemTable() ? HConstants.SYSTEMTABLE_QOS : 
HConstants.NORMAL_QOS);
+int priority = HConstants.NORMAL_QOS;
+if (tn != null && tn.isSystemTable()
+&& !tn.equals(SlowLogTableAccessor.SLOW_LOG_TABLE_NAME)) {

Review comment:
   Should we set the priority on the write req directly (Put) rather than 
having extra check like this at different places?  That will work right?





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

[GitHub] [hbase] anoopsjohn commented on a change in pull request #1681: HBASE-23938 : System table hbase:slowlog to store complete slow/large…

2020-05-18 Thread GitBox



anoopsjohn commented on a change in pull request #1681:
URL: https://github.com/apache/hbase/pull/1681#discussion_r427045601



##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/slowlog/LogEventHandler.java
##
@@ -53,12 +55,28 @@
 class LogEventHandler implements EventHandler {
 
   private static final Logger LOG = 
LoggerFactory.getLogger(LogEventHandler.class);
+  private static final int SYS_TABLE_QUEUE_SIZE = 1000;

Review comment:
   My ask was do u want to put a new config similar to the ring buffer's 
rather than hard coded 1000.
   After discussion, it looks like the confusion mostly coming because of these 
Q sizes and its value is not really going with its intent.  Ideally the 
ringbuffer Q is supposed to hold the slow logs for much longer time. The table 
Q is cleared/reduced in every 10 mins. So ideally the size of the table Q <= 
ring buffer Q size.  Then all confusions will go.
   I would suggest we do this way.
   We can have a new config like 'hbase.regionserver.slowlog.ringbuffer.size' 
for the table Q size. Its default value can be the value of 
hbase.regionserver.slowlog.ringbuffer.size' itself.   This 2 Q system will 
allow user to even have a tableQ size > ringbuffer Q size by tuning the config. 
 
   





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

[GitHub] [hbase] anoopsjohn commented on a change in pull request #1681: HBASE-23938 : System table hbase:slowlog to store complete slow/large…

2020-05-17 Thread GitBox



anoopsjohn commented on a change in pull request #1681:
URL: https://github.com/apache/hbase/pull/1681#discussion_r426227621



##
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java
##
@@ -1532,6 +1532,16 @@
 "hbase.regionserver.slowlog.buffer.enabled";
   public static final boolean DEFAULT_ONLINE_LOG_PROVIDER_ENABLED = false;
 
+  /** The slowlog info family as a string*/
+  private static final String SLOWLOG_INFO_FAMILY_STR = "info";
+
+  /** The slowlog info family */
+  public static final byte [] SLOWLOG_INFO_FAMILY = 
Bytes.toBytes(SLOWLOG_INFO_FAMILY_STR);
+
+  public static final String SLOW_LOG_SYS_TABLE_ENABLED_KEY =
+"hbase.regionserver.slowlog.systable.enabled";
+  public static final boolean DEFAULT_SLOW_LOG_SYS_TABLE_ENABLED_KEY = false;

Review comment:
   Are we not logging the region's encoded name?  This is the region name 
which comes as part of the req param:

##
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java
##
@@ -83,6 +83,12 @@
   public static final TableName META_TABLE_NAME =
   valueOf(NamespaceDescriptor.SYSTEM_NAMESPACE_NAME_STR, "meta");
 
+  /** hbase:slowlog table name - can be enabled
+   * with config - hbase.regionserver.slowlog.systable.enabled
+   */
+  public static final TableName SLOW_LOG_TABLE_NAME =

Review comment:
   Are we exposing this table name to customer? This is a public class.  If 
we expose the name for user to create queries, we might have to expose the 
column names also?  But those can not be done in this class. Any thinking of 
having a new Public class if needed to expose? If you dont want to expose this 
table name, we should not keep this in TableName public class

##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/slowlog/LogEventHandler.java
##
@@ -53,12 +55,28 @@
 class LogEventHandler implements EventHandler {
 
   private static final Logger LOG = 
LoggerFactory.getLogger(LogEventHandler.class);
+  private static final int SYS_TABLE_QUEUE_SIZE = 1000;
 
-  private final Queue queue;
+  private final Queue queueForRingBuffer;
+  private final Queue queueForSysTable;
+  private final boolean isSlowLogTableEnabled;
 
-  LogEventHandler(int eventCount) {
+  private Configuration configuration;
+
+  private static final ReentrantLock LOCK = new ReentrantLock();
+
+  LogEventHandler(int eventCount, boolean isSlowLogTableEnabled, Configuration 
conf) {
+this.configuration = conf;
 EvictingQueue evictingQueue = 
EvictingQueue.create(eventCount);
-queue = Queues.synchronizedQueue(evictingQueue);
+queueForRingBuffer = Queues.synchronizedQueue(evictingQueue);
+this.isSlowLogTableEnabled = isSlowLogTableEnabled;
+if (isSlowLogTableEnabled) {
+  EvictingQueue evictingQueueForTable = 
EvictingQueue.create(

Review comment:
   When system table logging is enabled, we have 2 queue.  This new one 
will always have 1000 max size where as the 'queueForRingBuffer' will have a 
default of 256 only.  Anyways we will occupy more heap memory upto 1000 
messages. Why we should reduce the max numbers in queueForRingBuffer?
   

##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/slowlog/LogEventHandler.java
##
@@ -53,12 +55,28 @@
 class LogEventHandler implements EventHandler {
 
   private static final Logger LOG = 
LoggerFactory.getLogger(LogEventHandler.class);
+  private static final int SYS_TABLE_QUEUE_SIZE = 1000;
 
-  private final Queue queue;
+  private final Queue queueForRingBuffer;
+  private final Queue queueForSysTable;
+  private final boolean isSlowLogTableEnabled;
 
-  LogEventHandler(int eventCount) {
+  private Configuration configuration;
+
+  private static final ReentrantLock LOCK = new ReentrantLock();
+
+  LogEventHandler(int eventCount, boolean isSlowLogTableEnabled, Configuration 
conf) {
+this.configuration = conf;
 EvictingQueue evictingQueue = 
EvictingQueue.create(eventCount);
-queue = Queues.synchronizedQueue(evictingQueue);
+queueForRingBuffer = Queues.synchronizedQueue(evictingQueue);
+this.isSlowLogTableEnabled = isSlowLogTableEnabled;
+if (isSlowLogTableEnabled) {
+  EvictingQueue evictingQueueForTable = 
EvictingQueue.create(

Review comment:
   Can we have a way to avoid this 2 queues but keep it single?

##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/slowlog/LogEventHandler.java
##
@@ -53,12 +55,28 @@
 class LogEventHandler implements EventHandler {
 
   private static final Logger LOG = 
LoggerFactory.getLogger(LogEventHandler.class);
+  private static final int SYS_TABLE_QUEUE_SIZE = 1000;

Review comment:
   You want a config for this like 
'hbase.regionserver.slowlog.ringbuffer.size'?

##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/slowlog/SlowLogTableAccessor.java
##

[GitHub] [hbase] anoopsjohn commented on a change in pull request #1681: HBASE-23938 : System table hbase:slowlog to store complete slow/large…

2020-05-15 Thread GitBox



anoopsjohn commented on a change in pull request #1681:
URL: https://github.com/apache/hbase/pull/1681#discussion_r425925567



##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/master/slowlog/SlowLogMasterService.java
##
@@ -0,0 +1,72 @@
+/*
+ *
+ * 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.master.slowlog;
+
+import java.io.IOException;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.MetaTableAccessor;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder;
+import org.apache.hadoop.hbase.client.TableDescriptorBuilder;
+import org.apache.hadoop.hbase.master.MasterServices;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Slowlog Master services - Table creation to be used by HMaster
+ */
+@InterfaceAudience.Private
+public class SlowLogMasterService {
+
+  private static final Logger LOG = 
LoggerFactory.getLogger(SlowLogMasterService.class);
+
+  private final boolean slowlogTableEnabled;
+  private final MasterServices masterServices;
+
+  private static final TableDescriptorBuilder TABLE_DESCRIPTOR_BUILDER =
+
TableDescriptorBuilder.newBuilder(TableName.SLOW_LOG_TABLE_NAME).setRegionReplication(1)
+  .setColumnFamily(
+
ColumnFamilyDescriptorBuilder.newBuilder(HConstants.SLOWLOG_INFO_FAMILY)
+  .setScope(HConstants.REPLICATION_SCOPE_LOCAL)
+  .setInMemory(true)

Review comment:
   We should make the blockCaching = false for this table?

##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/master/slowlog/SlowLogMasterService.java
##
@@ -0,0 +1,72 @@
+/*
+ *
+ * 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.master.slowlog;
+
+import java.io.IOException;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.MetaTableAccessor;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder;
+import org.apache.hadoop.hbase.client.TableDescriptorBuilder;
+import org.apache.hadoop.hbase.master.MasterServices;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Slowlog Master services - Table creation to be used by HMaster
+ */
+@InterfaceAudience.Private
+public class SlowLogMasterService {
+
+  private static final Logger LOG = 
LoggerFactory.getLogger(SlowLogMasterService.class);
+
+  private final boolean slowlogTableEnabled;
+  private final MasterServices masterServices;
+
+  private static final TableDescriptorBuilder TABLE_DESCRIPTOR_BUILDER =
+
TableDescriptorBuilder.newBuilder(TableName.SLOW_LOG_TABLE_NAME).setRegionReplication(1)
+  .setColumnFamily(
+
ColumnFamilyDescriptorBuilder.newBuilder(HConstants.SLOWLOG_INFO_FAMILY)
+  .setScope(HConstants.REPLICATION_SCOPE_LOCAL)
+  .setInMemory(true)

Review comment:
   Why we need inMemory true ?

##
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/TableName.java
##
@@ -83,6 +83,12 @@
   public static final TableName META_TABLE_NAME =
   valueOf(NamespaceDescriptor.SYSTEM_NAMESPACE_NAME_STR, "meta");
 
+  /** hbase:slowlog

[GitHub] [hbase] anoopsjohn commented on a change in pull request #1681: HBASE-23938 : System table hbase:slowlog to store complete slow/large…

[GitHub] [hbase] anoopsjohn commented on a change in pull request #1681: HBASE-23938 : System table hbase:slowlog to store complete slow/large…

[GitHub] [hbase] anoopsjohn commented on a change in pull request #1681: HBASE-23938 : System table hbase:slowlog to store complete slow/large…

[GitHub] [hbase] anoopsjohn commented on a change in pull request #1681: HBASE-23938 : System table hbase:slowlog to store complete slow/large…

4 matches

Site Navigation

Mail list logo

Footer information