Re: [DISCUSS] FunctionAdapter incompatible serialVersionUID

[Jason Huynh <jh...@pivotal.io>]

I've sent in a pull request for this:
https://github.com/apache/geode/pull/1119

I've added the old serialVersionUID to the FunctionAdapter, under the
assumption that anyone in 1.0-1.3 would have seen that the FunctionAdapter
had been deprecated and not used it.  The 1.0-1.3 users could also easily
change "extends FunctionAdapter" to "implements Function", recompile and
things would work for them.  The users pre-1.0 would have a slightly more
difficult approach.

Anyone interested, please review/accept or reject the pull request.

Thanks,
-Jason

On Wed, Nov 29, 2017 at 9:09 AM Darrel Schneider <ds...@pivotal.io>
wrote:

> +1 to not removing deprecated apis in minor releases.
> The semver policy Alexander describes seems reasonable.
> In the past of we have had something deprecated for a long time we have
> felt free to remove it whenever we want but I think the semver policy is a
> better way to decide when we are free to remove deprecated external apis.
>
>
> On Wed, Nov 29, 2017 at 8:50 AM, Alexander Murmann <am...@pivotal.io>
> wrote:
>
> > Even though the class is deprecated, you should be able to go from one
> > minor version to another without having to worry about anything breaking.
> > The point of semver is to provide information about things breaking or
> not
> > without having to read the changelog. If we remove APIs in a minor
> version
> > because they were previously deprecated we break that contract.
> Semver.org
> > <https://semver.org/#how-should-i-handle-deprecating-functionality>
> > recommends
> > that the functionality should be marked as deprecated in a minor release
> > and then removed as part of a major release.
> >
> > On Tue, Nov 28, 2017 at 7:03 PM, Jacob Barrett <jb...@pivotal.io>
> > wrote:
> >
> > > Since the class was deprecated and is technically only there for
> pre-1.0
> > > compatibly then the behavior of this class should be consistent with
> the
> > > pre-1.0 version. This will break 1.0 to 1.3 but anyone coding to a
> > post-1.0
> > > version should not be using this deprecated class.
> > >
> > >
> > > > On Nov 28, 2017, at 5:22 PM, Jason Huynh <jh...@pivotal.io> wrote:
> > > >
> > > > *With your proposoal 1.0 - 1.3 users would have modify their source
> > code
> > > on
> > > > the client and the server forthe function, correct?*
> > > >
> > > > If they start a new geode server 1.4+ and happened to extend
> > > > functionAdapter (it was deprecated in 1.0) then they would have to
> > > > recompile their client to not use functionAdapter.
> > > >
> > > > This should only affect users that extend FunctionAdapter and execute
> > > > functions by serializing them to the server from the client.  If they
> > > > execute by id it should not run into this problem...
> > > >
> > > >> On Tue, Nov 28, 2017 at 5:20 PM Jason Huynh <jh...@pivotal.io>
> > wrote:
> > > >>
> > > >> Dan, yeah, the suggested change in the stack overflow answer does
> work
> > > and
> > > >> I was able to put an if with the exact serialVersionUid before
> posting
> > > the
> > > >> proposal, but it is pretty hacky and may affect another class that
> > > somehow
> > > >> generated the same uid.  I can make that change too but I'd prefer
> not
> > > to
> > > >> have to maintain it moving forward...
> > > >>
> > > >>
> > > >>
> > > >>> On Tue, Nov 28, 2017 at 5:09 PM Dan Smith <ds...@pivotal.io>
> wrote:
> > > >>>
> > > >>> I agree I don't think we can get rid of FunctionAdapter until the
> > next
> > > >>> major version.
> > > >>>
> > > >>> I was thinking FunctionAdapter is rather widely used, but then I'm
> > > >>> surprised no one has hit this yet.
> > > >>>
> > > >>> All of the options kinda suck here - either pre 1.0 users have a
> > > >>> compatibility issue or 1.0-1.3 users do. With your proposoal 1.0 -
> > 1.3
> > > >>> users would have modify their source code on the client and the
> > server
> > > for
> > > >>> the function, correct?
> > > >>>
> > > >>> If we got really fancy we could actually ignore the
> serialVersionUUID
> > > for
> > > >>> this class like this - https://stackoverflow.com/a/1816711/2813144
> .
> > > But
> > > >>> it's pretty messy.
> > > >>>
> > > >>> -Dan
> > > >>>
> > > >>> On Tue, Nov 28, 2017 at 1:59 PM, Alexander Murmann <
> > > amurmann@pivotal.io>
> > > >>> wrote:
> > > >>>
> > > >>>> Anil, I am not sure following. I think FunctionAdapter already is
> > > >>>> deprecated. Isn't it? Anthony is right though that we shouldn't
> > remove
> > > >>>> anything customer facing unless we are doing a major release.
> > > Otherwise
> > > >>> we
> > > >>>> are violating the contract provided by semantic versioning.
> > > >>>>
> > > >>>> On Tue, Nov 28, 2017 at 1:52 PM, Anilkumar Gingade <
> > > agingade@pivotal.io
> > > >>>>
> > > >>>> wrote:
> > > >>>>
> > > >>>>> I haven't seen many uses of FunctionAdapter; if its not used
> much,
> > I
> > > >>>> think
> > > >>>>> we should deprecate this...
> > > >>>>>
> > > >>>>> It only provided default implementation for few of the methods;
> > this
> > > >>>> could
> > > >>>>> be added in the docs/release notes to help application to move to
> > > >>>> function
> > > >>>>> implementation.
> > > >>>>>
> > > >>>>> -Anil.
> > > >>>>>
> > > >>>>>
> > > >>>>> On Tue, Nov 28, 2017 at 12:40 PM, Anthony Baker <
> abaker@pivotal.io
> > >
> > > >>>> wrote:
> > > >>>>>
> > > >>>>>> I think we should wait for a major release to remove API’s.  If
> we
> > > >>>> broke
> > > >>>>> a
> > > >>>>>> public API, we should fix that IMO.
> > > >>>>>>
> > > >>>>>> Anthony
> > > >>>>>>
> > > >>>>>>
> > > >>>>>>> On Nov 28, 2017, at 11:40 AM, Patrick Rhomberg <
> > > >>> prhomberg@pivotal.io
> > > >>>>>
> > > >>>>>> wrote:
> > > >>>>>>>
> > > >>>>>>> +1 to removing a long-deprecated class from the Geode side.
> > > >>>>>>>
> > > >>>>>>> On Tue, Nov 28, 2017 at 8:04 AM, Bruce Schuchardt <
> > > >>>>>> bschuchardt@pivotal.io>
> > > >>>>>>> wrote:
> > > >>>>>>>
> > > >>>>>>>> How about just getting rid of this class?  After all it was
> > > >>> marked
> > > >>>> as
> > > >>>>>>>> being deprecated in 1.0.  Pivotal could add a compatible
> > > >>>>> FunctionAdapter
> > > >>>>>>>> class in their GemFire builds to support these old clients.
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>> On 11/27/17 10:18 AM, Jason Huynh wrote:
> > > >>>>>>>>>
> > > >>>>>>>>> This is a discussion for the fix to GEODE-4008:
> > > >>>>>>>>> InvalidClassException when deserializing FunctionAdapter from
> > > >>> pre
> > > >>>>> Geode
> > > >>>>>>>>> clients
> > > >>>>>>>>>
> > > >>>>>>>>> There was a change to deprecate FunctionAdapter in Geode
> > (before
> > > >>>>> 1.0),
> > > >>>>>> and
> > > >>>>>>>>> this also removed the method signatures in the class. This
> > > >>> caused
> > > >>>>> Java
> > > >>>>>> to
> > > >>>>>>>>> generate a new serialVersionUID to the class because one was
> > not
> > > >>>>>> assigned
> > > >>>>>>>>> previously. However we have clients pre Geode that when they
> > > >>>> attempt
> > > >>>>> to
> > > >>>>>>>>> execute a function by serializing the function across (not
> > > >>> using a
> > > >>>>>>>>> function
> > > >>>>>>>>> id), the FunctionAdapter class is unable to deserialize
> > > >>> properly.
> > > >>>>>>>>>
> > > >>>>>>>>> The proposed fix is to assign a serialVersionUID to the class
> > > >>> that
> > > >>>>>> matches
> > > >>>>>>>>> that of the pre Geode FunctionAdapter. This will cause any
> > Geode
> > > >>>>>> 1.0-1.3
> > > >>>>>>>>> clients to now run into the error but the older clients would
> > > >>> work
> > > >>>>>> fine.
> > > >>>>>>>>> Because FunctionAdapter has been deprecated it should be easy
> > > >>>> enough
> > > >>>>>> for
> > > >>>>>>>>> Geode 1.0-1.3 users to change their custom classes to
> implement
> > > >>>>>> Function
> > > >>>>>>>>> directly and not use the deprecated FunctionAdapter class.
> > > >>>>>>>>>
> > > >>>>>>>>> Please let me know if there is a better solution or if there
> > are
> > > >>>>>> problems
> > > >>>>>>>>> with the proposed fix.
> > > >>>>>>>>>
> > > >>>>>>>>>
> > > >>>>>>>>> Thanks,
> > > >>>>>>>>>
> > > >>>>>>>>> -Jason
> > > >>>>>>>>>
> > > >>>>>>>>>
> > > >>>>>>>>
> > > >>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>>
> > > >>>
> > > >>
> > >
> >
>