I left some minor comments on github - latest patch looks cleaner. Nice progress.
On Thu, Feb 6, 2014 at 7:23 AM, Lukas Nalezenec < [email protected]> wrote: > On 6.2.2014 14:08, Ted Yu wrote: > >> Have you considered putting the check inside a static method of >> RegionSizeCalculator ? >> > No, I havent. > I can do it but we will have to add this check to each use of > RegionSizeCalculator. There is risk of unnecessary NPEs. > I prefer readable, simple code over avoiding allocation of few kilobytes > for hundreds of milliseconds in noncritical code in case somebody turns the > feature off once a year. > I will add the static method but i wont use it in the InputFormat. > > Btw, maybe the best option is removing the configuration option ;-) - its > really fast now. > > >> Did you put latest patch into use in a cluster ? >> > Yes, just now. it looks pretty good. Its fast and its correlated with > previous Filesystem algorithm. > > https://dl.dropboxusercontent.com/u/77461754/tableSplitReturns0.png > > It ignores column families but it works with memStore. > > I will write unit tests and then make patch. > > Best regards > Lukas > > >> Thanks >> >> >> On Feb 6, 2014, at 4:48 AM, Lukas Nalezenec < >> [email protected]> wrote: >> >> Right, the instantiation could be skipped but I would prefer keeping it >>> as it is. >>> This was my intentional design decision - all functionality should be >>> encapsulated inside class. >>> Lukas >>> >>> On 3.2.2014 19:37, Ted Yu wrote: >>> >>>> The new config is checked inside RegionSizeCalculator ctor. >>>> Instantiation of RegionSizeCalculator can be skipped if the config says >>>> disabled, right ? >>>> >>> >
