hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From st...@duboce.net
Subject Re: Review Request: Reconstruction log playback has no bounds on memory used
Date Mon, 14 Jun 2010 22:28:47 GMT


> On 2010-06-14 14:14:55, Todd Lipcon wrote:
> > Regarding rewriting recovered edits back to the WAL, I'm afraid we may have an issue
with the following situation:
> > - RS A writes edits 1-100 to log
> > - RS A fails
> > - RS B starts recovering log, rewrites edits 1-50
> > - RS B fails while recovering
> > - RS C starts up. It *should* replay 1-100. Instead I fear it might only replay
1-50, or replay 1-100 followed by 1-50, either of which would be incorrect.
> > 
> > We should add a test case where the RS fails in the middle of recovering a prior
failure.
> > 
> > 
> > Also, replayRecoveredEdits returns the highest seqid from the logs. Is it possible
for storeFiles to have a higher seqid than what's in the log in any situation? We should add
some kind of assert perhaps.

On your scenario, yes, there is a hole.  Sorry, meant to point it out.  Was going to file
issue after this went in (Previous to this patch we'd have lost all 1-100 so this is at least
an improvement).  Currently, there is no means for C to startup and replay 1-100.  It has
no access to the recovered edits that B was processing.  As is, it'll replay 1-50 from the
split of B's WAL log.  The splitter needs to look for recovered.edits files and merge them
in for C on startup.


"Is it possible for storeFiles to have a higher seqid than what's in the log in any situation?"

Should not happen but going to add check for it and use the max from store files if its larger
than whats in logs.  Let me add test for this too.


> On 2010-06-14 14:14:55, Todd Lipcon wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 359
> > <http://review.hbase.org/r/179/diff/1/?file=1335#file1335line359>
> >
> >     this returns -1 if there is nothing to replay. Shouldn't it return the maxSeqId
+ 1 perhaps?
> >     
> >     Do we have a test which makes a region, writes some data, closes the region,
opens the region, writes some data, then recovers logs?

Yes, it should return the largest of maxSeqId and replayRecoveredEdits.  Let me add test.

No test that writes edits into a (multifamily region), then opens it again and recovers (I
made comment in test that we need such a thing).  Let me add a test to do this.  It'll likely
flush out more sequenceid issues.


> On 2010-06-14 14:14:55, Todd Lipcon wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 378
> > <http://review.hbase.org/r/179/diff/1/?file=1335#file1335line378>
> >
> >     we used to print the sequence ID after the increment, now we print before the
increment. Maybe change log to "sequenceid for next edit will be NNN"

Done


> On 2010-06-14 14:14:55, Todd Lipcon wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1904
> > <http://review.hbase.org/r/179/diff/1/?file=1335#file1335line1904>
> >
> >     should probably use LOG.error for these

done


> On 2010-06-14 14:14:55, Todd Lipcon wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1952
> > <http://review.hbase.org/r/179/diff/1/?file=1335#file1335line1952>
> >
> >     I agree, i don't think the splitter would ever write a partial one. We should
treat an IOE here as a real error

done


> On 2010-06-14 14:14:55, Todd Lipcon wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1978
> > <http://review.hbase.org/r/179/diff/1/?file=1335#file1335line1978>
> >
> >     when would this happen? deserves a comment I think

done (should never happen -- maybe schema gets changed between crash and replay?  I added
this because in original j-d test he had some messing adding edit for family not in schema...
I kept that messing and added a handler here.  If it goes into HRegion#put/#delete, it'll
cause dirty exception).


> On 2010-06-14 14:14:55, Todd Lipcon wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1985
> > <http://review.hbase.org/r/179/diff/1/?file=1335#file1335line1985>
> >
> >     can we rename maxSeqIdInLog to currentEditSeqId or something? since that's what
we're really talking about here. Also reversing this inequality to: if (currentEditSeqId <
storeMaxSeqId>) I think it would read more clearly

done


> On 2010-06-14 14:14:55, Todd Lipcon wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 2017
> > <http://review.hbase.org/r/179/diff/1/?file=1335#file1335line2017>
> >
> >     I think this will crash when the region contains stores that were the result
of an HFOF bulk load (they have no sequence id)

You mean it will throw: throw new IllegalAccessError("Has not been initialized");?  Lets fix
this.  Is this your bug from today?


> On 2010-06-14 14:14:55, Todd Lipcon wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 2026
> > <http://review.hbase.org/r/179/diff/1/?file=1335#file1335line2026>
> >
> >     maybe rename to restoreEdit() or something to be clear that this is during restoration
only?

done


> On 2010-06-14 14:14:55, Todd Lipcon wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 2032
> > <http://review.hbase.org/r/179/diff/1/?file=1335#file1335line2032>
> >
> >     possibly more efficient:
> >     Collections.singletonMap(kv.getFamily(), Collections.singletonList(kv))
> >     
> >     though there may be a comparator issue?

Yeah, comparator issue for Map (used your singletonList suggestion though).


> On 2010-06-14 14:14:55, Todd Lipcon wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/Store.java, line 241
> > <http://review.hbase.org/r/179/diff/1/?file=1337#file1337line241>
> >
> >     comment is now out of date, right?

done


> On 2010-06-14 14:14:55, Todd Lipcon wrote:
> > src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java, line 850
> > <http://review.hbase.org/r/179/diff/1/?file=1340#file1340line850>
> >
> >     maybe just Preconditions.checkArgument here so it throws if you try to pass
a non-DFS?

done


- stack


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/179/#review216
-----------------------------------------------------------


On 2010-06-14 11:43:30, stack wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/179/
> -----------------------------------------------------------
> 
> (Updated 2010-06-14 11:43:30)
> 
> 
> Review request for hbase.
> 
> 
> Summary
> -------
> 
> + Moved replay of edits up from Store up into Region. This means we play the edits once
only rather than once per Store.
> + Lots of cleanup in the replay of edits code. Uses the new flag introduced by cosmin
– hbase.skip.errors. If set, and exception processing edits, we'll fail. If false, we'll
move the broke edits file aside and keep going. Renamed the method from doReconstructionLog
to replayRecoveredEdits (reconstruction?).
> + The main change in this patch is that recovering edits, we now go in via the HRegion
main API doing put and delete so that only one code path, so replayed edits are added to the
WAL, and so a flush will be triggered if we fill memory.
> + In HStore, lots of removed code and comments since no longer does log replay. Cleanup
of maximum seqid. Calculate it instead rather than save as a data member. Its only used once
on HRegion startup.
> + Change the HRegion#initialize. It used to take 'initial files' which is a notion never
used (it was a means of putting files in place after a split but split is done internal to
HRegion so can do things in HRegions guts w/o need of exposing notion of initial files). I
removed it and added overload that takes no args which is the usual way this method is invoked.
> + Rename the product of splits, 'recovered.edits' instead of 'oldlogfile.log'
> + Added small facility to HBaseTestingUtility for creating different user in a Configuration
so can have more than one Filesystem instance the easier.
> + Redid the test TestStoreReconstruction as TestWALReplay.
> 
> 
> This addresses bug hbase-1025.
>     http://issues.apache.org/jira/browse/hbase-1025
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/hadoop/hbase/HConstants.java f5d3e94 
>   src/main/java/org/apache/hadoop/hbase/HMerge.java 62f3561 
>   src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 06e022c 
>   src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java bca819e 
>   src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 2a0dcee 
>   src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 05cf17f 
>   src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java dbb21d4 
>   src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 479c661 
>   src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 43a8a28 
>   src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreReconstruction.java 4f0417d

>   src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java 86cf4ea 
>   src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestWALReplay.java PRE-CREATION

> 
> Diff: http://review.hbase.org/r/179/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> stack
> 
>


Mime
View raw message