> On 2010-11-11 06:23:59, Kannan Muthukkaruppan wrote: > > Last major should be tracked on per region+CF basis, rather than just a per > > region basis, right? > > > > Nicolas's & Amit's recent changes to compaction algorithm already makes it > > such that different column families within the same region could end up > > having their major compactions done at different times. [A minor which > > includes all files will implicitly get promoted to a major]. > > > > Is there some boolean state/flag in the HFile's meta data already that > > tells us if the file was the result of a major compaction or not? > > > > If so, isn't it sufficient to use the timestamp of the file which has the > > "major compaction" tag to determine when a particular region+CF was last > > major compacted? > > > > stack wrote: > Kannan, yes, there is a flag on end of HFile already which says if file > is result of a major compaction and yes, it would make more sense to do this > on a per-Store level (though up to this its been at the Region level). And > thanks, your note reminds me that all is different since the new compaction > code went in. I'm thinking the latter probably even 'fixes' the original > issue. Lets see. Meantime, I think we should move this issue out of 0.90. > We can then take the time with the implementation.
I think I'm -1 on moving out of 0.90. This is an actual bug and others have already -1'd the punting from 0.90 when I tried to do that earlier :) @Kannan, I don't see how Amit's change allows majors to be done on one family but not the other. This is a change from Nicolas' patch (though it's actually undocumented from what I read), but looking at the code it does upgrade to major if all files selected. In any case, I don't actually think that's necessarily a deal breaker. This check is only for periodic major compaction checking. I would say the expected behavior there is that if one family did get a minor turned into major but the other didn't, you would respect the lastMajor of the family that didn't get the previous major compaction upgrade. (Periodic major must happen on all stores for now while we live in the world of this stuff being done at region level not store level). So it wouldn't break that case. The case that it's one family, or all families get upgraded, then you would be working off of an 'old' lastMajor stamp, yes. However, at the least, you would avoid doing anything crazy like re-major compacting a single file. There are checks that even if the period is expired, if there's only one file and it's the result of a major compaction, then don't do the major. Once a new file did show up in at least one store, then a periodic major compaction would happen, yes. If we think this is a big deal, I could think of one additional way to mitigate. We make the decision to do a major compaction at the Store level (the periodic major compact check calls to the region, the region iterates the stores and if any returns yes then it does a major). We could bake in to the Store check, in addition to looking at the region-level lastMajor, looking at the current files of the store for any that are major compacted. If there is a major compacted file, and it is more recent than the region-level lastMajor, then we use that stamp to check against the period expiration check. I think that would work. lastMajorStamp = Math.max(regionLevelLastMajor, lastMajorFromFilesOfThisStore)... if the filesFromThisStore don't have any major compacted files, then will always use regionLevelLastMajor which is the fix for HBASE-2990. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1089/#review1906 ----------------------------------------------------------- On 2010-11-10 17:04:39, Jonathan Gray wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.cloudera.org/r/1089/ > ----------------------------------------------------------- > > (Updated 2010-11-10 17:04:39) > > > Review request for hbase, stack and khemani. > > > Summary > ------- > > This is a somewhat misguided attempt. It's not done but it shows the fairly > simple change to the actual major compaction code. > > The hard part is: > > + long lastMajor = region.getRegionInfo().getRegionData().getLastMajor(); > > And the question is where to persist that. > > This patch adds a new class called HRegionData into HRegionInfo that contains > any number of key-value pairs of data that get persisted with the HRI. Not > really sure how I ended up there but this data seemed like an odd-man-out so > adding another field seemed weird. We also need some kind of versioning in > HRI so we can add stuff w/o migrating. There's some versioned stuff in > HRData. > > Just looking for some feedback / ideas. > > > This addresses bugs HBASE-2990 and HBASE-3083. > http://issues.apache.org/jira/browse/HBASE-2990 > http://issues.apache.org/jira/browse/HBASE-3083 > > > Diffs > ----- > > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java > 1033780 > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1033780 > > Diff: http://review.cloudera.org/r/1089/diff > > > Testing > ------- > > > Thanks, > > Jonathan > >
