> On June 30, 2014, 3:24 p.m., kturner wrote: > > server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java, > > line 267 > > <https://reviews.apache.org/r/22658/diff/2/?file=619604#file619604line267> > > > > would be interesting to see performance diff w/ this change and > > ACCUMULO-2801 > > > > Jonathan Park wrote: > Bump on the comments on that ticket :)
I made some comments on it. > On June 30, 2014, 3:24 p.m., kturner wrote: > > server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java, > > line 262 > > <https://reviews.apache.org/r/22658/diff/2/?file=619604#file619604line262> > > > > Maybe this is a problem in the existing code, but this change may make > > the problem worse. Calling beginUpdatingLogsUsed() updates the tablets in > > memory data structs to indicate the tablet is using the log. If the code > > after this point fails, then all tablets will think the walog is in the > > metadata table even though it is not. > > > > > > Jonathan Park wrote: > I guess you're suggesting that if we failed for whatever reason while > holding the lock, our changes shouldn't be published outside of the lock > (since all readers of the wal in memory structs also lock). > > I think we could either implement some kind of rollback or convince > ourselves that it's harmless (or implement it so that it is harmless). I'll > give this some thought. > > kturner wrote: > Maybe the tablet data structs should be updated in > finishUpdatingLogsUsed() after the metadata updates are made successfully. > Then have to consider the case where walog ref ends up in metadata table but > not in tablet data structs. Seems like this case is easier to prevent and > should it occur it seems innocuous. I opened ACCUMULO-2960 to track this possible issue. - kturner ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22658/#review46980 ----------------------------------------------------------- On June 27, 2014, 5:41 p.m., Jonathan Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22658/ > ----------------------------------------------------------- > > (Updated June 27, 2014, 5:41 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-2889 > https://issues.apache.org/jira/browse/ACCUMULO-2889 > > > Repository: accumulo > > > Description > ------- > > Added additional methods to the Writer class to handle batching. > > Potential risks are that we're now holding onto the locks for a bit longer > than we used to. All tablets present in a batch will have their logLock's > locked until the batch is complete. > > > Diffs > ----- > > core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java d6762e7 > > server/base/src/main/java/org/apache/accumulo/server/util/InternalBatchWriter.java > PRE-CREATION > > server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java > 374017d > server/tserver/src/main/java/org/apache/accumulo/tserver/Tablet.java > 36b2289 > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java > 57415bd > > server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java > d25ee75 > > test/src/test/java/org/apache/accumulo/server/util/InternalBatchWriterIT.java > PRE-CREATION > test/src/test/java/org/apache/accumulo/test/BatchMetadataUpdatesIT.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/22658/diff/ > > > Testing > ------- > > Added a new IT. > > We insert a few entries to tablets and ensure that the relevant entries > appear in the WAL and Metadata Table. > One test case that isn't included yet is verifying that root + metadata table > entries are entered correctly. > > > Thanks, > > Jonathan Park > >
