----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32224/#review77098 -----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/tabletserver/log/LogEntry.java <https://reviews.apache.org/r/32224/#comment124953> As a follow-on, this would be a good class to get some explicit unit test coverage for (since it acts less like an internal struct with your changes) core/src/main/java/org/apache/accumulo/core/tabletserver/log/LogEntry.java <https://reviews.apache.org/r/32224/#comment124954> Have you tested out a dirty 1.6 shutdown and starting up 1.7 with these changes in place? server/base/src/main/java/org/apache/accumulo/server/master/state/TabletLocationState.java <https://reviews.apache.org/r/32224/#comment124955> just delete it server/base/src/main/java/org/apache/accumulo/server/replication/StatusUtil.java <https://reviews.apache.org/r/32224/#comment124956> It would be better to remove the synchronized block and make a new builder each time. This is more in line with how PB is intended to be used. server/base/src/main/java/org/apache/accumulo/server/util/MasterMetadataUtil.java <https://reviews.apache.org/r/32224/#comment124957> Add a better msg if you intend to keep it in place server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java <https://reviews.apache.org/r/32224/#comment124959> StringBuilder instead? Would be more apparent when we have (more) bugs like the one where we missed a slash in a hand-concat'ed path server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java <https://reviews.apache.org/r/32224/#comment124961> It's unlikely to be hit, but a finally would be nice here too. server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java <https://reviews.apache.org/r/32224/#comment124960> Need finally to close these. server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java <https://reviews.apache.org/r/32224/#comment124962> Need finally server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java <https://reviews.apache.org/r/32224/#comment124964> A few sets of unnecessary parens server/gc/src/main/java/org/apache/accumulo/gc/replication/CloseWriteAheadLogReferences.java <https://reviews.apache.org/r/32224/#comment124966> Get rid of these or move them to trace server/master/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java <https://reviews.apache.org/r/32224/#comment124970> I think this would be deserving a log message in the negative. Would help to know that the realized its state its up to date. server/tserver/src/main/java/org/apache/accumulo/server/GarbageCollectionLogger.java <https://reviews.apache.org/r/32224/#comment124971> This looks like it should be done in its own commit server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java <https://reviews.apache.org/r/32224/#comment124972> Would help to be more clear as to why this is a warning. A user/admin wouldn't be able to parse what the msg means. server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java <https://reviews.apache.org/r/32224/#comment124973> This block seems a bit dubious. Why do all calls to this method for non-root/meta loggers need to be synchronized? If the synchronization is intentional, some javadoc/comments would be greatly appreciated. server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java <https://reviews.apache.org/r/32224/#comment124975> Why is DfsLogger Comparable now? Isn't the log name still just a UUID? server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java <https://reviews.apache.org/r/32224/#comment124979> Don't you want to do this in the finally one more level "out" (and only if +oldName+ is non-null)? server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java <https://reviews.apache.org/r/32224/#comment124981> Better represented as (numAdded == 0 || numAdded == 1) - Josh Elser On March 19, 2015, 4:42 p.m., Eric Newton wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32224/ > ----------------------------------------------------------- > > (Updated March 19, 2015, 4:42 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-3423 > https://issues.apache.org/jira/browse/ACCUMULO-3423 > > > Repository: accumulo > > > Description > ------- > > Faster WAL rollovers > > > Diffs > ----- > > > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java > 6a5c74a > core/src/main/java/org/apache/accumulo/core/conf/Property.java 2403915 > core/src/main/java/org/apache/accumulo/core/metadata/RootTable.java 24148b1 > > core/src/main/java/org/apache/accumulo/core/metadata/schema/MetadataSchema.java > 534dd7f > core/src/main/java/org/apache/accumulo/core/tabletserver/log/LogEntry.java > 25d0f32 > > core/src/test/java/org/apache/accumulo/core/metadata/MetadataTableSchemaTest.java > PRE-CREATION > server/base/src/main/java/org/apache/accumulo/server/fs/VolumeUtil.java > fc26ca4 > server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java > e5bdded > > server/base/src/main/java/org/apache/accumulo/server/master/state/MetaDataStateStore.java > 7ee6f0c > > server/base/src/main/java/org/apache/accumulo/server/master/state/MetaDataTableScanner.java > 9be5a67 > > server/base/src/main/java/org/apache/accumulo/server/master/state/TabletLocationState.java > b24b562 > > server/base/src/main/java/org/apache/accumulo/server/master/state/TabletStateStore.java > 5413e31 > > server/base/src/main/java/org/apache/accumulo/server/master/state/ZooTabletStateStore.java > ab99396 > > server/base/src/main/java/org/apache/accumulo/server/replication/StatusUtil.java > 898e3d4 > > server/base/src/main/java/org/apache/accumulo/server/util/ListVolumesUsed.java > e90d1dd > > server/base/src/main/java/org/apache/accumulo/server/util/MasterMetadataUtil.java > 4ca2d64 > > server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java > 10cd749 > > server/base/src/main/java/org/apache/accumulo/server/util/ReplicationTableUtil.java > af02a8d > > server/base/src/test/java/org/apache/accumulo/server/util/ReplicationTableUtilTest.java > b1010c2 > > server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java > 1735c0d > server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java > c8d5cd6 > > server/gc/src/main/java/org/apache/accumulo/gc/replication/CloseWriteAheadLogReferences.java > 3a32727 > > server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogsTest.java > 5801faa > > server/gc/src/test/java/org/apache/accumulo/gc/replication/CloseWriteAheadLogReferencesTest.java > 23db83a > server/master/src/main/java/org/apache/accumulo/master/Master.java 3762f32 > > server/master/src/main/java/org/apache/accumulo/master/MasterClientServiceHandler.java > f73c236 > > server/master/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java > d097d75 > > server/master/src/main/java/org/apache/accumulo/master/replication/WorkMaker.java > 8532e1b > > server/master/src/main/java/org/apache/accumulo/master/state/MergeStats.java > 44f229e > > server/master/src/test/java/org/apache/accumulo/master/ReplicationOperationsImplTest.java > a127dcd > server/master/src/test/java/org/apache/accumulo/master/TestMergeState.java > b0240f1 > > server/master/src/test/java/org/apache/accumulo/master/state/RootTabletStateStoreTest.java > abceae4 > server/tserver/src/main/findbugs/exclude-filter.xml 47dd1f5 > > server/tserver/src/main/java/org/apache/accumulo/server/GarbageCollectionLogger.java > 8f3785e > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletLevel.java > PRE-CREATION > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java > 662ee31 > server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java > 5acf5eb > > server/tserver/src/main/java/org/apache/accumulo/tserver/log/SortedLogRecovery.java > c4d9fab > > server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java > 711c497 > > server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CommitSession.java > b4814e4 > > server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/DatafileManager.java > 594d9c5 > server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java > 6152500 > > server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/TabletCommitter.java > 4bc05a6 > > test/src/main/java/org/apache/accumulo/test/performance/thrift/NullTserver.java > b429607 > test/src/test/java/org/apache/accumulo/proxy/ProxyDurabilityIT.java 404a8fd > test/src/test/java/org/apache/accumulo/test/BadDeleteMarkersCreatedIT.java > 25337b2 > test/src/test/java/org/apache/accumulo/test/BalanceIT.java f793925 > test/src/test/java/org/apache/accumulo/test/CleanWalIT.java f553be8 > test/src/test/java/org/apache/accumulo/test/ConditionalWriterIT.java > bd00f02 > > test/src/test/java/org/apache/accumulo/test/MissingWalHeaderCompletesRecoveryIT.java > b78a311 > test/src/test/java/org/apache/accumulo/test/NoMutationRecoveryIT.java > 6a9975c > test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 56a6a70 > test/src/test/java/org/apache/accumulo/test/functional/WALSunnyDayIT.java > PRE-CREATION > > test/src/test/java/org/apache/accumulo/test/functional/WatchTheWatchCountIT.java > bd0555b > > test/src/test/java/org/apache/accumulo/test/performance/RollWALPerformanceIT.java > PRE-CREATION > > test/src/test/java/org/apache/accumulo/test/replication/GarbageCollectorCommunicatesWithTServersIT.java > 5b89d9c > > test/src/test/java/org/apache/accumulo/test/replication/MultiInstanceReplicationIT.java > 9dec16e > test/src/test/java/org/apache/accumulo/test/replication/ReplicationIT.java > 54348db > > Diff: https://reviews.apache.org/r/32224/diff/ > > > Testing > ------- > > Ran all tests, except RandomWalk. > > > Thanks, > > Eric Newton > >
