Fix multiple concurrency bugs in Master.gatherTableInformation() #546

keith-turner · 2018-07-02T17:39:55Z

Master.gatherTableInformation() had the following problems :

If Property.MASTER_STATUS_THREAD_POOL_SIZE set > 1, then multiple threads
would put into a TreeMap
Create a thread pool and never called shutdown now
Returns a reference to a treemap that threads in thread pool may still
be adding to.

This patch also attempts to address the issues brought up in #402 by switching
to a cached thread pool. This will allow the thread pool to expand so that
unresponsive tservers do not prevent gathering status from responsive tservers.

Master.gatherTableInformation() had the following problems : * If Property.MASTER_STATUS_THREAD_POOL_SIZE set > 1, then multiple threads would put into a TreeMap * Create a thread pool and never called shutdown now * Returns a reference to a treemap that threads in thread pool may still be adding to. This patch also attempts to address the issues brought up in apache#402 by switching to a cached thread pool. This will allow the thread pool to expand so that unresponsive tservers do not prevent gathering status from responsive tservers.

ctubbsii · 2018-07-02T18:18:47Z

core/src/main/java/org/apache/accumulo/core/conf/Property.java

@@ -340,6 +340,7 @@
  MASTER_REPLICATION_COORDINATOR_THREADCHECK("master.replication.coordinator.threadcheck.time",
      "5s", PropertyType.TIMEDURATION,
      "The time between adjustments of the coordinator thread pool"),
+  @Deprecated


Description should be updated to reflect when it was deprecated. I noticed while reviewing deprecated items that we hadn't been doing this for configuration properties, and it's very useful.

ctubbsii · 2018-07-02T18:28:11Z

server/master/src/main/java/org/apache/accumulo/master/Master.java

+      // Since an unbounded thread pool is being used, rate limit how fast task are added to the
+      // executor. This prevents the threads from growing large unless there are lots of
+      // unresponsive tservers.
+      sleepUninterruptibly(5, TimeUnit.MILLISECONDS);


I think maybe this should be some fraction of the client timeout, because the client timeout indicates some user awareness of how long the RPCs should take.

ctubbsii · 2018-07-02T18:39:20Z

server/master/src/main/java/org/apache/accumulo/master/Master.java

+
+    // Because result is a ConcurrentSkipListMap will not see a concurrent modification exception,
+    // even though background threads may still try to put.
+    SortedMap<TServerInstance,TabletServerStatus> info = ImmutableSortedMap.copyOf(result);


If something isn't in this map by the time we get here, we could just add the remaining directly to the badServers.

The bad server processing needs some attention, I will open up a follow on issue.

keith-turner · 2018-07-03T13:19:19Z

All ITs passed running against 8e29faa

ctubbsii

Reminder: the deprecated configuration property description should reflect which version it was deprecated since.

ctubbsii · 2018-07-04T02:15:50Z

core/src/main/java/org/apache/accumulo/core/conf/Property.java

@@ -340,6 +340,10 @@
  MASTER_REPLICATION_COORDINATOR_THREADCHECK("master.replication.coordinator.threadcheck.time",
      "5s", PropertyType.TIMEDURATION,
      "The time between adjustments of the coordinator thread pool"),
+  /**
+   * @deprecated since 1.9.1


I actually meant in the text in the property description. The javadoc comment here won't really be useful. Also, it should be 1.9.2.

oh yeah, should be be 1.9.2. Not exactly sure what you are looking for, can you give a code example?

See line 348 below. s/The number of threads/Deprecated since 1.9.2. The number of threads/
The @Deprecated annotation is sufficient for devs, but these String descriptions get published in our docs for users, where they are useful for both users and devs.

I updated the docs. I looked into the the code that generates prop docs and it generates documentation for deprecated props. So users looking at the docs will know which props are deprecated, but they will not know when. If ACCUMULO-4592 were done, could also add a deprecated version.

keith-turner · 2018-07-09T20:49:20Z

I was a bit uneasy with the unlimited thread pool, so I made the number of threads configurable again in 476db84. It defaults to an unlimited threads pool, but if that causes problems there is a config option to limit the thread pool size.

ctubbsii · 2018-07-09T22:20:28Z

server/master/src/main/java/org/apache/accumulo/master/Master.java

@@ -1146,12 +1148,20 @@ private long balanceTablets() {

  private SortedMap<TServerInstance,TabletServerStatus> gatherTableInformation(
      Set<TServerInstance> currentServers) {
+    final long rpcTimeout = getConfiguration().getTimeInMillis(Property.GENERAL_RPC_TIMEOUT);
+    int threads = Math.max(getConfiguration().getCount(Property.MASTER_STATUS_THREAD_POOL_SIZE), 0);


This line seems to imply that negative values will result in unlimited threads. However, the documentation only describes zero threads behaving this way. The docs should say "non-positive" or "zero and negative" just to leave no ambiguity.

That was not my intent. I will removed the max, then the following line would fail if negative.

keith-turner added 2 commits July 2, 2018 13:26

deprecated unused prop

91127f4

ctubbsii reviewed Jul 2, 2018

View reviewed changes

use general rpc timeout

8e29faa

ctubbsii requested changes Jul 3, 2018

View reviewed changes

code review update

6fa3f76

ctubbsii reviewed Jul 4, 2018

View reviewed changes

code review updates

7182b1a

ctubbsii approved these changes Jul 5, 2018

View reviewed changes

make num threads configurable again

476db84

ctubbsii reviewed Jul 9, 2018

View reviewed changes

code review update

34dce7b

keith-turner added v2.0.0 blocker This issue blocks any release version labeled on it. bug This issue has been verified to be a bug. labels Jul 10, 2018

keith-turner merged commit 42a3534 into apache:1.9 Jul 10, 2018

keith-turner deleted the master-concurrent-bug branch December 6, 2018 15:16

ctubbsii added this to Done in 1.9.2 Jun 14, 2019

ctubbsii added this to Done in 2.0.0 Jun 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix multiple concurrency bugs in Master.gatherTableInformation() #546

Fix multiple concurrency bugs in Master.gatherTableInformation() #546

keith-turner commented Jul 2, 2018

ctubbsii Jul 2, 2018

ctubbsii Jul 2, 2018

ctubbsii Jul 2, 2018

keith-turner Jul 2, 2018

keith-turner commented Jul 3, 2018

ctubbsii left a comment

ctubbsii Jul 4, 2018

keith-turner Jul 4, 2018

ctubbsii Jul 4, 2018 •

edited

Loading

keith-turner Jul 5, 2018

keith-turner commented Jul 9, 2018

ctubbsii Jul 9, 2018

keith-turner Jul 10, 2018

Fix multiple concurrency bugs in Master.gatherTableInformation() #546

Fix multiple concurrency bugs in Master.gatherTableInformation() #546

Conversation

keith-turner commented Jul 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith-turner commented Jul 3, 2018

ctubbsii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctubbsii Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith-turner commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctubbsii Jul 4, 2018 •

edited

Loading