drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacques Nadeau <jacq...@dremio.com>
Subject Re: Request - hold off on merging to master for 48 hours
Date Wed, 29 Jul 2015 19:17:26 GMT
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 <chriswestin42@gmail.com>
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
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message