> On March 7, 2014, 1:04 p.m., Mike Drob wrote: > > server/base/src/main/java/org/apache/accumulo/server/util/time/SimpleTimer.java, > > line 38 > > <https://reviews.apache.org/r/18909/diff/1/?file=513673#file513673line38> > > > > Logging both the message and the exception is generally duplicative, as > > the framework should print the message before the stack trace anyway.
Good point, will fix. > On March 7, 2014, 1:04 p.m., Mike Drob wrote: > > server/base/src/main/java/org/apache/accumulo/server/util/time/SimpleTimer.java, > > line 74 > > <https://reviews.apache.org/r/18909/diff/1/?file=513673#file513673line74> > > > > If I call getInstance(1) and later getInstance(2) then that behaviour > > is not clear from an API perspective. Properties beginning with general.* > > usually can't be changed at runtime though, so we might be ok. True. The behavior is that the later call will return the singleton instance with only one thread. I can add a log message warning about this case? I should also note it in the Javadoc. > On March 7, 2014, 1:04 p.m., Mike Drob wrote: > > server/base/src/main/java/org/apache/accumulo/server/util/time/SimpleTimer.java, > > line 132 > > <https://reviews.apache.org/r/18909/diff/1/?file=513673#file513673line132> > > > > I don't see this getting called anywhere. Do we need to retrofit > > existing classes to clean up after themselves? I added this anticipating a desire for it, but you are correct, it's not used. Actually, I added it before I changed the threads to be daemons, so I can remove it. > On March 7, 2014, 1:04 p.m., Mike Drob wrote: > > server/base/src/test/java/org/apache/accumulo/server/util/time/SimpleTimerTest.java, > > line 24 > > <https://reviews.apache.org/r/18909/diff/1/?file=513675#file513675line24> > > > > Add a test to check exceptions? Sure, I'll see what I can add. - Bill ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18909/#review36541 ----------------------------------------------------------- On March 7, 2014, 11:52 a.m., Bill Havanki wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18909/ > ----------------------------------------------------------- > > (Updated March 7, 2014, 11:52 a.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-2419 > https://issues.apache.org/jira/browse/ACCUMULO-2419 > > > Repository: accumulo > > > Description > ------- > > A conversion from SimpleTimer using java.util.Timer to > java.util.concurrent.ScheduledThreadPoolExecutor. > > Most of the key changes are in SimpleTimer.java. It now can support more than > one thread; the number of threads is specified either directly or through the > new general.server.simpletimer.threadpool.size configuration property. The > default is one thread, as with Timer. Submitted tasks are no longer decorated > with LoggingTimerTask, but run as-is, with a thread exception handler taking > over logging of errors. A unit test written before the conversion ensures > that SimpleTimer still works. > > Other changes are there to move away from calling the no-arg > SimpleTimer.getInstance(), so that the prevailing configuration is consulted > to figure out the SimpleTimer thread count. > > > Diffs > ----- > > core/src/main/java/org/apache/accumulo/core/conf/Property.java > efa7eb58f9d38ba6d68b70b502353ad5a8899ba2 > > minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloClusterImpl.java > 35a7b9d2889d34ef0d323713b004e9d1d3b25989 > server/base/src/main/java/org/apache/accumulo/server/Accumulo.java > 2fa905113caa583241cf88afc6c9d39a467bfedc > > server/base/src/main/java/org/apache/accumulo/server/master/LiveTServerSet.java > 63bd894e1e3122b11074074455be7fbfe5043736 > server/base/src/main/java/org/apache/accumulo/server/util/TServerUtils.java > 6d9e4c7735a4fe72312b2e5640dc8a59916cade1 > > server/base/src/main/java/org/apache/accumulo/server/util/time/SimpleTimer.java > 499b0de4f505d62ae48272c93a9901a0e6642746 > > server/base/src/main/java/org/apache/accumulo/server/zookeeper/DistributedWorkQueue.java > c5a952827e0944f62b227b10b0504af7ce9496be > > server/base/src/test/java/org/apache/accumulo/server/util/time/SimpleTimerTest.java > PRE-CREATION > server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java > 89925b46a77a978982208f8117f6ec38281450e3 > server/master/src/main/java/org/apache/accumulo/master/Master.java > 8e98c0421d219b34c0306465ae5f65fab7b94a00 > > server/master/src/main/java/org/apache/accumulo/master/recovery/RecoveryManager.java > 76d352059fa3bce0fff1370dc982fa56da50c450 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/BulkImport.java > bdc89dd1140ac1cba822f96c613112b1b5c9fa75 > > server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java > bf65dae1fe9dd0fab68be8b67965460bb4990809 > server/tracer/src/main/java/org/apache/accumulo/tracer/TraceServer.java > 30f1ae768a9c61464eda991eed94de8e74f39dca > > server/tserver/src/main/java/org/apache/accumulo/tserver/CompactionWatcher.java > e6ca38feff9920d10136540c7163cda2d5d3b1f8 > server/tserver/src/main/java/org/apache/accumulo/tserver/FileManager.java > bb95532589b6a5476459fadf54131b968882915d > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java > cdb9dbf015be231358af420a79231082a9566ac0 > > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java > db046e9da55ff66a223d34bb832c495157a25ee6 > server/tserver/src/main/java/org/apache/accumulo/tserver/log/LogSorter.java > 8f783c330acfcc7599468bf269fb70c8e2a828b7 > test/src/main/java/org/apache/accumulo/test/functional/ZombieTServer.java > f26c8d7fcaf9695f04a64b2668ab2d37afded0df > > test/src/main/java/org/apache/accumulo/test/performance/thrift/NullTserver.java > 0591b19b83a9a3a9b1cf62a80f967fa633a2520e > > Diff: https://reviews.apache.org/r/18909/diff/ > > > Testing > ------- > > SimpleTimer unit test succeeds. Ran functional tests, most succeeded (the few > failures are unrelated to SimpleTimer). Ran test cluster with changes, did > basic table operations. > > > Thanks, > > Bill Havanki > >
