accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Eric Newton" <eric.new...@gmail.com>
Subject Re: Review Request 32224: ACCUMULO-3423
Date Tue, 14 Apr 2015 22:04:01 GMT


> On March 27, 2015, 3:43 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/tabletserver/log/LogEntry.java, line
125
> > <https://reviews.apache.org/r/32224/diff/1/?file=899476#file899476line125>
> >
> >     should there be a sanity check on the length of `parts[]`?  Maybe the construction
should do the sanity check

Added a unit test for LogEntry, and check the format of the string a little more carefully.


> On March 27, 2015, 3:43 p.m., kturner wrote:
> > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java, line
1783
> > <https://reviews.apache.org/r/32224/diff/1/?file=899507#file899507line1783>
> >
> >     why not just remove this thrift call?

[~elserj] wanted it in there.


> On March 27, 2015, 3:43 p.m., kturner wrote:
> > server/master/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java,
line 292
> > <https://reviews.apache.org/r/32224/diff/1/?file=899498#file899498line292>
> >
> >     The code to merge tablets had checks to ensure it would not merge tablets with
walogs.  Need to ensure the updates to add walogs to a tablet are always made before those
checks.   Looking at the code, it seems like this may be the case but I am not completely
sure.   Maybe some test for this situation?  Maybe shard and bulk random walk with agitation
is enought to test this situation.  Do you think ITs are needed?

The only way to get unassigned by the master is to get the list of WALogs added at the same
time.


> On March 27, 2015, 3:43 p.m., kturner wrote:
> > server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java,
line 210
> > <https://reviews.apache.org/r/32224/diff/1/?file=899510#file899510line210>
> >
> >     I think the following code to create a log could be removed with the SynchronousQueue...
just always take from the syncronous queue

Good catch.  That improved the code a great deal.


> On March 27, 2015, 3:43 p.m., kturner wrote:
> > server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java,
line 243
> > <https://reviews.apache.org/r/32224/diff/1/?file=899510#file899510line243>
> >
> >     why not just move check up to beginning and bail if there is already a next
log?

Used the SynchronousQueue as suggested.


> On March 27, 2015, 3:43 p.m., kturner wrote:
> > server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java,
line 240
> > <https://reviews.apache.org/r/32224/diff/1/?file=899510#file899510line240>
> >
> >     Could iterate through tablets and find levels present, then pass those levels
to `addLoggersToMetadata()`.  This would avoid synchronizing on each tablet.

Good idea.


> On March 27, 2015, 3:43 p.m., kturner wrote:
> > server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java,
line 245
> > <https://reviews.apache.org/r/32224/diff/1/?file=899510#file899510line245>
> >
> >     Is this expected to occur often?  Should this unused log be marked unused in
 metadata table?

Code has been restructured using the SynchronousQueue, and this warning will never occure.


- Eric


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32224/#review78049
-----------------------------------------------------------


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