accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Josh Elser" <josh.el...@gmail.com>
Subject Re: Review Request 32224: ACCUMULO-3423
Date Thu, 19 Mar 2015 20:45:36 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message