[GitHub] [hbase] Joseph295 commented on a change in pull request #1783: HBASE-24436 The store file open and close thread pool should be share…

2020-05-29 Thread GitBox


Joseph295 commented on a change in pull request #1783:
URL: https://github.com/apache/hbase/pull/1783#discussion_r432411820



##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##
@@ -590,7 +588,7 @@ void setDataBlockEncoderInTest(HFileDataBlockEncoder 
blockEncoder) {
 }
   }
 } finally {
-  storeFileOpenerThreadPool.shutdownNow();
+

Review comment:
   > If nothing to cleanup, remove the finally block?
   
   Ok, let me update the patch a moment later





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] Joseph295 commented on a change in pull request #1783: HBASE-24436 The store file open and close thread pool should be share…

2020-05-27 Thread GitBox


Joseph295 commented on a change in pull request #1783:
URL: https://github.com/apache/hbase/pull/1783#discussion_r431092284



##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##
@@ -955,19 +964,24 @@ private void bulkLoadHFile(HStoreFile sf) throws 
IOException {
   if (!result.isEmpty()) {
 // initialize the thread pool for closing store files in parallel.
 ThreadPoolExecutor storeFileCloserThreadPool = this.region
-.getStoreFileOpenAndCloseThreadPool("StoreFileCloser-"
-  + this.region.getRegionInfo().getEncodedName() + "-" + 
this.getColumnFamilyName());
-
+.getStoreFileOpenAndCloseThreadPool();
+String executingThreadNamePrefix = "StoreFileCloser-"
+  + this.region.getRegionInfo().getEncodedName() + "-" + 
this.getColumnFamilyName();
+AtomicInteger threadId = new AtomicInteger(0);
 // close each store file in parallel
 CompletionService completionService =
   new ExecutorCompletionService<>(storeFileCloserThreadPool);
 for (HStoreFile f : result) {
   completionService.submit(new Callable() {
 @Override
 public Void call() throws IOException {
+  Thread t = Thread.currentThread();

Review comment:
   > I dont think we should do these hacks. The threads can be named with 
regionName and some seqNo. Anyway even if we do these hacks and add the cf 
name, it says till that level only. Still from jstack we can not know which 
file open it is stuck with (if assume we took it when there was a stuck case). 
We can make sure we have proper log which says opening store file xxx for cf x 
and that log will have to say the thread name also. So later with jstack and 
this log we can track down issues. Sounds ok?
   
   Yeah, definitely, let me update the patch soon.





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] Joseph295 commented on a change in pull request #1783: HBASE-24436 The store file open and close thread pool should be share…

2020-05-27 Thread GitBox


Joseph295 commented on a change in pull request #1783:
URL: https://github.com/apache/hbase/pull/1783#discussion_r430881933



##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##
@@ -695,6 +695,7 @@ void sawNoSuchFamily() {
   private RegionCoprocessorHost coprocessorHost;
 
   private TableDescriptor htableDescriptor = null;
+  private ThreadPoolExecutor storeFileOpenAndCloseThreadPool;

Review comment:
   > Why is this shared at the Region level. Shouldn't it be at the Store 
level (in HStore)?
   > 
   > What does sharing at the region level gain us? Are you attempting to 
evenly round-robin store open work over all opening stores in the region? Just 
sharing an executor at region level won't do this. If the underlying stores are 
skewed the order in which runnables are submitted to the executor will share 
the skew.
   
   If only one store file is too slow and that is the bottleneck of the whole 
open or close region process, it seems not easy to fix that by threadpool 
here(maybe hedged read?). As Anoops said above, the problem we want to solve 
here is some stores may have too many store files, while others have much 
fewer. It will be quicker ideally if  all of the store files just share the 
whole thread pool in one region.





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] Joseph295 commented on a change in pull request #1783: HBASE-24436 The store file open and close thread pool should be share…

2020-05-27 Thread GitBox


Joseph295 commented on a change in pull request #1783:
URL: https://github.com/apache/hbase/pull/1783#discussion_r431050865



##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##
@@ -955,19 +964,24 @@ private void bulkLoadHFile(HStoreFile sf) throws 
IOException {
   if (!result.isEmpty()) {
 // initialize the thread pool for closing store files in parallel.
 ThreadPoolExecutor storeFileCloserThreadPool = this.region
-.getStoreFileOpenAndCloseThreadPool("StoreFileCloser-"
-  + this.region.getRegionInfo().getEncodedName() + "-" + 
this.getColumnFamilyName());
-
+.getStoreFileOpenAndCloseThreadPool();
+String executingThreadNamePrefix = "StoreFileCloser-"
+  + this.region.getRegionInfo().getEncodedName() + "-" + 
this.getColumnFamilyName();
+AtomicInteger threadId = new AtomicInteger(0);
 // close each store file in parallel
 CompletionService completionService =
   new ExecutorCompletionService<>(storeFileCloserThreadPool);
 for (HStoreFile f : result) {
   completionService.submit(new Callable() {
 @Override
 public Void call() throws IOException {
+  Thread t = Thread.currentThread();

Review comment:
   > All these looks bit hacky
   
   What do you think about here, Anoop? Just remove the name related logic? Or 
just set and not reset ?





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] Joseph295 commented on a change in pull request #1783: HBASE-24436 The store file open and close thread pool should be share…

2020-05-27 Thread GitBox


Joseph295 commented on a change in pull request #1783:
URL: https://github.com/apache/hbase/pull/1783#discussion_r430934955



##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##
@@ -1769,7 +1771,7 @@ public void setTimeoutForWriteLock(long 
timeoutForWriteLock) {
   storeCloserThreadPool.shutdownNow();
 }
   }
-
+  storeFileOpenAndCloseThreadPool.shutdownNow();

Review comment:
   
   > This means keeping this 'storeFileOpenAndCloseThreadPool' active till the 
Region is closed.
   > previously we create the pool at the region open time and once the stores 
are opened, the pool is shutdown. Keeping it longer this way not looks good
   
   There is a timeout for this thread pool, after some time, the threads will 
be destroyed.





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] Joseph295 commented on a change in pull request #1783: HBASE-24436 The store file open and close thread pool should be share…

2020-05-27 Thread GitBox


Joseph295 commented on a change in pull request #1783:
URL: https://github.com/apache/hbase/pull/1783#discussion_r430933648



##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##
@@ -955,19 +964,24 @@ private void bulkLoadHFile(HStoreFile sf) throws 
IOException {
   if (!result.isEmpty()) {
 // initialize the thread pool for closing store files in parallel.
 ThreadPoolExecutor storeFileCloserThreadPool = this.region
-.getStoreFileOpenAndCloseThreadPool("StoreFileCloser-"
-  + this.region.getRegionInfo().getEncodedName() + "-" + 
this.getColumnFamilyName());
-
+.getStoreFileOpenAndCloseThreadPool();
+String executingThreadNamePrefix = "StoreFileCloser-"
+  + this.region.getRegionInfo().getEncodedName() + "-" + 
this.getColumnFamilyName();
+AtomicInteger threadId = new AtomicInteger(0);
 // close each store file in parallel
 CompletionService completionService =
   new ExecutorCompletionService<>(storeFileCloserThreadPool);
 for (HStoreFile f : result) {
   completionService.submit(new Callable() {
 @Override
 public Void call() throws IOException {
+  Thread t = Thread.currentThread();

Review comment:
   
   > All these looks bit hacky
   
   Changing the thread name is not necessary, but it will be useful when jstack 
 the process, we will know something in detail.Yeah, it's bit hacky





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] Joseph295 commented on a change in pull request #1783: HBASE-24436 The store file open and close thread pool should be share…

2020-05-27 Thread GitBox


Joseph295 commented on a change in pull request #1783:
URL: https://github.com/apache/hbase/pull/1783#discussion_r430934059



##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##
@@ -555,15 +555,24 @@ void setDataBlockEncoderInTest(HFileDataBlockEncoder 
blockEncoder) {
 }
 // initialize the thread pool for opening store files in parallel..
 ThreadPoolExecutor storeFileOpenerThreadPool =
-  this.region.getStoreFileOpenAndCloseThreadPool("StoreFileOpener-"
-+ this.region.getRegionInfo().getEncodedName() + "-" + 
this.getColumnFamilyName());
+  this.region.getStoreFileOpenAndCloseThreadPool();
 CompletionService completionService =
   new ExecutorCompletionService<>(storeFileOpenerThreadPool);
-
+String executingThreadNamePrefix = "StoreFileOpener-"
+  + this.region.getRegionInfo().getEncodedName() + "-" + 
this.getColumnFamilyName();
+AtomicInteger threadId = new AtomicInteger(0);
 int totalValidStoreFile = 0;
 for (StoreFileInfo storeFileInfo : files) {
   // open each store file in parallel
-  completionService.submit(() -> 
this.createStoreFileAndReader(storeFileInfo));
+  completionService.submit(() -> {
+Thread t = Thread.currentThread();
+String name = t.getName();
+Thread.currentThread().setName(executingThreadNamePrefix + "-"

Review comment:
   Yes





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] Joseph295 commented on a change in pull request #1783: HBASE-24436 The store file open and close thread pool should be share…

2020-05-27 Thread GitBox


Joseph295 commented on a change in pull request #1783:
URL: https://github.com/apache/hbase/pull/1783#discussion_r430933648



##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
##
@@ -955,19 +964,24 @@ private void bulkLoadHFile(HStoreFile sf) throws 
IOException {
   if (!result.isEmpty()) {
 // initialize the thread pool for closing store files in parallel.
 ThreadPoolExecutor storeFileCloserThreadPool = this.region
-.getStoreFileOpenAndCloseThreadPool("StoreFileCloser-"
-  + this.region.getRegionInfo().getEncodedName() + "-" + 
this.getColumnFamilyName());
-
+.getStoreFileOpenAndCloseThreadPool();
+String executingThreadNamePrefix = "StoreFileCloser-"
+  + this.region.getRegionInfo().getEncodedName() + "-" + 
this.getColumnFamilyName();
+AtomicInteger threadId = new AtomicInteger(0);
 // close each store file in parallel
 CompletionService completionService =
   new ExecutorCompletionService<>(storeFileCloserThreadPool);
 for (HStoreFile f : result) {
   completionService.submit(new Callable() {
 @Override
 public Void call() throws IOException {
+  Thread t = Thread.currentThread();

Review comment:
   Changing the thread name is not necessary, but it will be useful when 
jstack  the process, we will know something in detail.Yeah, it's bit hacky
   
   > All these looks bit hacky
   
   Changing the thread name is not necessary, but it will be useful when jstack 
 the process, we will know something in detail.Yeah, it's bit hacky





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] Joseph295 commented on a change in pull request #1783: HBASE-24436 The store file open and close thread pool should be share…

2020-05-27 Thread GitBox


Joseph295 commented on a change in pull request #1783:
URL: https://github.com/apache/hbase/pull/1783#discussion_r430881933



##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##
@@ -695,6 +695,7 @@ void sawNoSuchFamily() {
   private RegionCoprocessorHost coprocessorHost;
 
   private TableDescriptor htableDescriptor = null;
+  private ThreadPoolExecutor storeFileOpenAndCloseThreadPool;

Review comment:
   > Why is this shared at the Region level. Shouldn't it be at the Store 
level (in HStore)?
   > 
   > What does sharing at the region level gain us? Are you attempting to 
evenly round-robin store open work over all opening stores in the region? Just 
sharing an executor at region level won't do this. If the underlying stores are 
skewed the order in which runnables are submitted to the executor will share 
the skew.
   
   If only one store is too slow and that is the bottleneck of the whole open 
or close region process, it seems not easy to fix that by threadpool here(maybe 
hedged read?). As Anoops said above, the problem we want to solve here is some 
stores may have too many store files, while others have much fewer. It will be 
quicker ideally if  all of the store files just share the whole thread pool in 
one region.





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] Joseph295 commented on a change in pull request #1783: HBASE-24436 The store file open and close thread pool should be share…

2020-05-27 Thread GitBox


Joseph295 commented on a change in pull request #1783:
URL: https://github.com/apache/hbase/pull/1783#discussion_r430876977



##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##
@@ -695,6 +695,7 @@ void sawNoSuchFamily() {
   private RegionCoprocessorHost coprocessorHost;
 
   private TableDescriptor htableDescriptor = null;
+  private ThreadPoolExecutor storeFileOpenAndCloseThreadPool;

Review comment:
   > If I understand the jira correctly, what you are trying to solve is 
below case.
   > One region with say 2 stores. Store1 having much more files than other. 
Say the config for the #threads in open pool is 10. Now it will create 2 pools 
for each store with 5 threads each. The Store2 will get finished soon. But 
store1 will take much longer. So if it was a shared pool of 10 threads the 
overall time for opening both stores would have been lesser. my understanding 
correct?
   
   Yes, just that case.





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