Re: Re: [pgjdbc] XADataSource support for resource sharing & interleaving. (#47) - Mailing list pgsql-jdbc

Dave Cramer <pg@fastcrypt.com> wrote:

> I'm looking for opinions on this patch.
>
> Heikki ? Kevin Grittner ?

To introduce myself to the POLARIS folks and put my comments in
perspective, I'm a committer on the underlying database product,
and spent a few years coding in Java -- including debugging a few
small JDBC issues.  I'm going to approach this more-or-less as I
would a review of source code for the core product, and Dave may
reign me in if the approach here is different.

First off, I would like to thank the POLARIS team for tackling this
and having the patience to work with the community in sharing your
work.  I promise we're not trying to be difficult, but a rigorous
process is important to the long-term viability of the product, and
I hope to keep the pain to a minimum.  My initial points may seem
picky, but experience has shown that these things do matter.

One things is the means of delivery.  It's great to use github to
develop "in the open".  I do that and applaud your use of this
technique.  The core developers, however, have a policy of asking
the submitter to email a patch to the community list.  The creates
a permanent record of who submitted what, when, and is considered
to be proof that you are offering the code to the community under
the terms of the project's license.  INAL, but apparently this is
considered important to avoid disputes about the right to
distribute the work.  So, based on the subsequent discussion, I
request that someone authorized to offer this to the community
submit a context or unified diff of the work to this list, as an
attachment to an email.

In comparing your github repository to the master branch, I see a
lot of whitespace changes that are not functionally necessary, and
I see changes to support relocation of the source code.  I know you
already moved the source code itself back to its historical
location, but the changes to support making it relocatable should
really be submitted as a separate patch, and whitespace changes
should also be submitted in a patch which does absolutely nothing
but modify whitespace and maybe comments.  As a reviewer I can say
that it helps tremendously to be able to focus on just the single
feature without these other "distractions".

Personally, I like the changes to make the source code relocatable.
Perhaps you could pull out just those changes as a fairly small
patch and submit that for discussion, review, and possible
inclusion in the community code?  That will be once step to keeping
the main patch smaller and more focused.

Then the whitespace changes.  In the core code we prohibit trailing
whitespace in source code.  If you run `git diff
master...POLARIS_XA` you will see the trailing whitespace in red.
If you could eliminate that, I would appreciate it.  If you could
then break out any whitespace changes (like bringing an opening
brace up from its own line to the end of the previous line) as a
separate patch, that would likewise minimize distractions in
reviewing the main patch.  Might as well do that first, too, so
whitespace in the main part of the patch can be in matching style.
There may be some discussion about how things *should* be
formatted; in the core project we have a piece of software that
standardizes all of that and saves a lot of arguments on such
issues.  Here we may have to go through a (hopefully brief)
religious war on the topic.

Of course, none of this is intended to substitute for a review of
the substance of the patch, which will take me a bit longer -- it
is intended to make review easier and more focused and allow those
looking back through the revision history to better understand the
changes.

Thanks again for tackling an ambitious feature set!

--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company