Return-Path: X-Original-To: apmail-accumulo-dev-archive@www.apache.org Delivered-To: apmail-accumulo-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 50CF317CC9 for ; Tue, 24 Mar 2015 16:38:11 +0000 (UTC) Received: (qmail 32060 invoked by uid 500); 24 Mar 2015 16:38:11 -0000 Delivered-To: apmail-accumulo-dev-archive@accumulo.apache.org Received: (qmail 32021 invoked by uid 500); 24 Mar 2015 16:38:11 -0000 Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@accumulo.apache.org Delivered-To: mailing list dev@accumulo.apache.org Received: (qmail 32008 invoked by uid 99); 24 Mar 2015 16:38:10 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 24 Mar 2015 16:38:10 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 4DD591D302D; Tue, 24 Mar 2015 16:38:10 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============6144139191312958579==" MIME-Version: 1.0 Subject: Re: Review Request 32224: ACCUMULO-3423 From: keith@deenlo.com To: "Eric Newton" , "accumulo" , keith@deenlo.com Date: Tue, 24 Mar 2015 16:38:10 -0000 Message-ID: <20150324163810.1505.85053@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: noreply@reviews.apache.org X-ReviewGroup: accumulo X-ReviewRequest-URL: https://reviews.apache.org/r/32224/ X-Sender: noreply@reviews.apache.org References: <20150319164257.19861.83455@reviews.apache.org> In-Reply-To: <20150319164257.19861.83455@reviews.apache.org> Reply-To: keith@deenlo.com X-ReviewRequest-Repository: accumulo --===============6144139191312958579== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32224/#review77582 ----------------------------------------------------------- I am still reviewing. I think I have covered Master, GC code, and a little bit of tserver code. Still want to take a more indepth look at tserver code and test. server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java I think catching this exception and continuing will cause the candidate to be removed from metatadata table. Is this what you wanted? Could possibly leave it in metadata table, so it would be a candidate again on the next GC run. server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java I think `removeReferencedCandidates()` or `removeInUseCandidates()` would be a better name for this. If this method name is changed, could rename `removeMarkers()` to `removeMetadataEntries()` :) server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java Feel like using UUIDs would be safer server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java TODO for me. I have not reviewed replication stuff yet. Wanted to discuss w/ you and get a quick conceptual overview first. server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java This code should be extemely mistrustful of the data its reading. `CurrentLogsSection.getTabletServer()` does some sanity checks of the data. What other sanity checks could be done here? Seems like some validation that the paths is as expected could be done. Also if strong validation of the tserver instance data is not, then it would be nice to validate that. server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java whats the purpose of `!rootWALs.contains(path)` server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java Not sure, I think the old code only dealt with UUIDs for determining what was in use. It seems like the chanes are comparing paths and not uuids? Comparing UUIDs was important for relatavie paths and also paths that are normalized differently. Maybe using hadoop will normalize things the same.. and maybe relative paths are no longer a concern? If so should check. Thinkin using only UUIDs for determiing in use status is still safest, sometimes two properly normalized paths can still be different but refer to the same thing. - kturner 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 > > --===============6144139191312958579==--