My thought is more general...why chase master? Let's get the algorithm and infrastructure right on some branch. Then I'm all for pausing for rebase purposes. There isn't enough fundamental stuff changing that this should me a giant issue. There is no reason we can't be 99% approved based off the 1.0 release branch. Then worry about the difference. If some things, like renaming and code organization, are cosmetic, get those merged asap. On Jul 29, 2015 1:59 PM, "Parth Chandra" <[email protected]> wrote:
> I think the idea (some of the idea is mine I'm afraid) is to allow Chris to > catch up and rebase, not to have it reviewed and merged in two days. > At the moment the problem is that every time he rebases, some test breaks > and while he's chasing that down, the master moves ahead. > If we get this 2 day break, we can get close enough to master and share the > changes (a pre-review perhaps). > Also, agree that perhaps the patch could be split into smaller pieces. Not > renaming the allocator would in fact save several files from being included > in the patch. > > > P > > > On Wed, Jul 29, 2015 at 12:17 PM, Jacques Nadeau <[email protected]> > wrote: > > > In general, this type of request makes a lot of sense. That said, we > > should get to +1 before we hold master. > > > > The last changes that were posted on DRILL-1942 are more than a month > old. > > The patch touched ~100 files and was thousands of lines. It had an issue > > that we identified. Since then, you exposed very little to the community > > on your progress. It seems unrealistic to provide a large update to this > > patch and expect review and merge within 48 hours. Had you been > exposing & > > discussing your work over the last month and the community agreed that it > > was ready for merge, your ask here would seem more feasible. > > > > My suggestion is to stop chasing master AND break your patch into smaller > > pieces. Looking at the old patch, the vast majority of changes have very > > little to do with a new allocator functionality. For example, renaming > the > > allocator could be merged without changing the underlying implementation > > (that would substantially reduce the patch size). > > > > For the allocator specifically, let's get the code right first. Then we > > can work as a community to minimize the effort to get it merged. > > > > > > > > -- > > Jacques Nadeau > > CTO and Co-Founder, Dremio > > > > On Wed, Jul 29, 2015 at 11:41 AM, Chris Westin <[email protected]> > > wrote: > > > > > I've got a large patch that includes a completely rewritten direct > memory > > > allocator (replaces TopLevelAllocator). > > > > > > The space accounting is much tighter than with the current > > implementation, > > > and it catches a lot more problems than the current implementation > does. > > It > > > also fixes issues with accounting around the use of shared buffers, and > > > buffer ownership transfer (used by the RPC layer to hand off buffers to > > > fragments that will do work). > > > > > > It's been an ongoing battle to get this in, because every time I get > > close, > > > I rebase, and it finds more new problems (apparently introduced by > other > > > work done since my last rebase). These take time to track down and fix, > > > because they're often in areas of the code I don't know. > > > > > > It looks like I'm very close right now. I rebased against apache/master > > on > > > Friday. All the unit tests passed. All of our internal tests passed > > except > > > for one query, which takes an IllegalReferenceCountException (it looks > > like > > > a DrillBuf is being released one more time than it should be). > > > > > > So, in order to keep the gap from getting wide again (it looks like I'm > > > already a couple of commits behind, but hopefully they don't introduce > > more > > > issues), I'm asking that folks hold off on merging into master for 48 > > hours > > > from now -- that's until about noon on Friday PST. I'm hoping that will > > > give me the time needed to finally get this in. If things go wrong with > > my > > > current patching, or I discover other problems, or can't find the > illegal > > > reference count issue by then, I'll post a message and open things up > > > again. Meanwhile, you can still pull, do work, make pull requests, and > > get > > > them reviewed; just don't merge them to master. > > > > > > Can we agree to this? > > > > > > Chris > > > > > >
