> On Oct 5, 2018, at 1:15 AM, Nico Williams <nico.willi...@twosigma.com> wrote:
>
> On Thu, Oct 04, 2018 at 11:19:06AM +0100, Alan Bateman wrote:
>> On 03/10/2018 21:49, Nico Williams wrote:
>>> :
>>> A lot of these changes are interrelated. Reviewing them in order of
>>> size might require rebasing our stack of commits, and may not be
>>> entirely possible.
>>>
>>> We're extremely familiar with this code as we have been patching the
>>> JGSS stack this way for years (we have developed these patches for JDKs
>>> 7, 8, 9, 10, 11, and the current 12 master), and we have been running
>>> this in production (with JDKs 7, 8, and 9, and soon 11)
>> Just a few high-level points on the patches that you attached:
>>
>> 1. It's important to take sponsor/Reviewer effort into account. I
>> skimmed through some of the 25 patches and they lack a detailed
>> description on what the issue is about. JGSS gurus might recognize
>> some of these issues from the diffs but I suspect you (or Victor)
>> will need to match the patches to existing issues in JIRA or else
>> get bugs submitted so that there is a description for each issue in
>> the bug database.
>
> We've pointed out that we need bugs filed. We'll work with Weijun to
> get them filed when he's back from vacation.
I'm just back. Will start working tomorrow.
>
>> 2. I skimmed the patches and didn't see any tests or changes to
>> existing tests. This may come up in the discussion of each change as
>> the default position is for all bug fixes should have tests where
>> feasible.
>
> That's an existing problem in the codebase. The tests are very limited
> as it is, and testing GSS/Kerberos requires mocking up a KDC, which
> requires more complex test infrastructure.
I do have a simple one at
http://hg.openjdk.java.net/jdk/jdk/file/tip/test/jdk/sun/security/krb5/auto/KDC.java
you can read OneKDC.java, Context.java and Basic.java in the same directory.
--Max
>
>> 3. I see the patches include at least some API changes so the
>> sponsor will need to submit a CSR for approval. API changes are only
>> allowed in feature releases.
>
> That's fine.
>
> Nico
> --
