geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ryan McMahon <rmcma...@pivotal.io>
Subject Re: Backwards compatibility issue with JSONFormatter
Date Wed, 15 May 2019 00:06:49 GMT
Fixed in
https://github.com/apache/geode/commit/cc0b37a504480f554b1884491f44a3cca4113ef5

On Tue, May 14, 2019 at 2:06 PM Ryan McMahon <rmcmahon@pivotal.io> wrote:

> Thanks Anthony!  Have we considered using a compliance checker in our CI
> like the one in the first link?
>
> Side note - I think that varargs is an interesting case that I don't see
> called out in those links.  Since varargs is just syntactic sugar, it is
> ultimately the varargs parameter is converted to an array in the bytecode.
> So in the JSON formatter change:
>
> fromJSON(byte[] jsonByteArray, String... identityFields)
>
>
> becomes
>
> fromJSON(byte[] jsonByteArray, String[] identityFields)
>
>
> Hence the breakage of the ABI.
>
> Ryan
>
> On Tue, May 14, 2019 at 1:33 PM Anthony Baker <abaker@pivotal.io> wrote:
>
>> Here are a few links on API compatibility:
>>
>> https://lvc.github.io/japi-compliance-checker/#Examples
>> https://wiki.eclipse.org/Evolving_Java-based_APIs
>> https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html
>>
>>
>> Anthony
>>
>>
>> > On May 14, 2019, at 12:45 PM, Dan Smith <dsmith@pivotal.io> wrote:
>> >
>> > Sounds good! Yeah, this is a bit of an interesting case where code will
>> > still compile against the new API without change, but I think
>> maintaining
>> > compatibility of the binary API is also important. Thanks for looking
>> into
>> > it.
>> >
>> > -Dan
>> >
>> > On Tue, May 14, 2019 at 11:22 AM Ryan McMahon <rmcmahon@pivotal.io>
>> wrote:
>> >
>> >> Hi all,
>> >>
>> >> This is my mistake - it was a misunderstanding of what constitutes a
>> >> breaking public API change.  If a client app were to recompile using
>> the
>> >> new bits with the new method signature, there wouldn't be an issue
>> because
>> >> the new signature would work with 0 varargs.  The problem arises when
>> you
>> >> hot swap the geode dependencies for that client app without
>> recompiling, as
>> >> the bytecode no longer matches.  Since we do support the hot swap use
>> case
>> >> for CVEs etc, I see now this is considered a breaking API change.
>> >>
>> >> I'll change this to use a method overload instead, it should be a
>> pretty
>> >> simple fix.  Luckily, this issue didn't make it into any Geode
>> releases.
>> >>
>> >> Thanks,
>> >> Ryan
>> >>
>> >>
>> >> On Tue, May 14, 2019 at 10:47 AM Udo Kohlmeyer <udo@apache.org> wrote:
>> >>
>> >>> How did this slip the review process that the signature of a public
>> API
>> >>> was changed?
>> >>>
>> >>> Well done Gester for finding this!!
>> >>>
>> >>> --Udo
>> >>>
>> >>> On 5/14/19 10:40, Dan Smith wrote:
>> >>>> I think the changes for GEODE-6327 broke backwards compatibility
in
>> >>>> JSONFormatter with the change from fromJSON(String jsonString) to
>> >>>> fromJSON(String jsonString, String... identityFields)
>> >>>>
>> >>>> Adding an additional varargs parameter to the method breaks code
that
>> >> was
>> >>>> compiled against the non-varargs version. I think we need to overload
>> >> the
>> >>>> method instead.
>> >>>>
>> >>>> Thanks to Gester for discovering this with his test!
>> >>>>
>> >>>> -Dan
>> >>>>
>> >>>
>> >>
>>
>>

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