[DISCUSS] Using Guava in Flink "core" packages

[Aljoscha Krettek-2]

Hi,

I recently saw that we added a dependency on our shaded-guava to flink-core [1]. Just for the record, I don’t want do diminish the contributions of anyone involved in the PR in any way. It just made me realise that we have some implicit agreements or assumptions about adding certain things to core packages that we might never have really discussed. I think we should do that now.

Quite some time ago an effort was started to reduce our dependency on Guava [2] because of some problems with version stability and dependency conflicts. At some later point, we created shaded guava so that we could use it without clashes [3]. I believe we now have shaded Guava only in runtime modules where it wasn’t easy to remove and CEP.

With the creation of shaded guava, I think we are in a bit of a limbo situation where it is not exactly clear what our stance towards it is, because it is easy to add to modules, as evident by the aforementioned PR. I think we should discuss that situation and agree upon a common stance on the topic.

In general, I think the surface (which includes classes, interfaces, and dependencies, among other things) of core modules should be kept as lean as possible.  (all modules really)

What do you think?

Best,
Aljoscha

[1] https://github.com/apache/flink/pull/7679 <https://github.com/apache/flink/pull/7679>
[2] https://issues.apache.org/jira/browse/FLINK-3700 <https://issues.apache.org/jira/browse/FLINK-3700>
[3] https://issues.apache.org/jira/browse/FLINK-6982

[Chesnay Schepler-3]

I fully agree that we should write down this kind of conventions that
committers have setup implicitly.

I agree that we should keep flink-core as lean as possible.

On guava usages in general my current stance is that we should only use
guava if necessary. Spreading it (or other dependencies for that matter)
to far just creates headaches when bumping the dependency.

One core principle that we *must* follow is that shaded dependencies are
neither exposed to user-code nor contained in the user-jar. Otherwise we
end up again in a situation where we cannot increment dependencies
without breaking compatibility for users.

The PR at hand has a few issues in this regard. The guava limiter is
only used in tests but is not a test class, yet isn't annotated with
either Public(Evolving)/Internal. As it stands I cannot judge whether
this is truly supposed to be an example / test utility or actually a
user-facing class.
If this is to be used by connectors, which are included in the user-jar,
then we're violating the principle above, in which case the class should
be relocated/removed.

On 06.03.2019 15:10, Aljoscha Krettek wrote:

> Hi,
>
> I recently saw that we added a dependency on our shaded-guava to flink-core [1]. Just for the record, I don’t want do diminish the contributions of anyone involved in the PR in any way. It just made me realise that we have some implicit agreements or assumptions about adding certain things to core packages that we might never have really discussed. I think we should do that now.
>
> Quite some time ago an effort was started to reduce our dependency on Guava [2] because of some problems with version stability and dependency conflicts. At some later point, we created shaded guava so that we could use it without clashes [3]. I believe we now have shaded Guava only in runtime modules where it wasn’t easy to remove and CEP.
>
> With the creation of shaded guava, I think we are in a bit of a limbo situation where it is not exactly clear what our stance towards it is, because it is easy to add to modules, as evident by the aforementioned PR. I think we should discuss that situation and agree upon a common stance on the topic.
>
> In general, I think the surface (which includes classes, interfaces, and dependencies, among other things) of core modules should be kept as lean as possible.  (all modules really)
>
> What do you think?
>
> Best,
> Aljoscha
>
> [1] https://github.com/apache/flink/pull/7679 <https://github.com/apache/flink/pull/7679>
> [2] https://issues.apache.org/jira/browse/FLINK-3700 <https://issues.apache.org/jira/browse/FLINK-3700>
> [3] https://issues.apache.org/jira/browse/FLINK-6982

[Timo Walther-2]

Hi,

yes I also fully agree that it is time to write down all these implicit
convensions that we've learned throught the last years. The Flink
community is growing quite rapidly right now and we must ensure that the
same mistakes do not repeat again.

Keeping the number of dependencies low is a very good example. Even if
Guava is shaded, some libraries also depend on Guava and maybe only
support a certain set of versions. Calcite is a good example. They use
Guava heavily and must even note which Guava versions they currently
maintain in the release notes [1] (currently 19-27). In most cases there
are core Java alternatives or one method must simply be added to some
utility class instead.

Stephan, me and other people work on code quality guide. We will publish
this soon to the ML for discussion.

Regards,
Timo

[1] https://calcite.apache.org/docs/history.html#v1-18-0

Am 06.03.19 um 15:40 schrieb Chesnay Schepler:

> I fully agree that we should write down this kind of conventions that
> committers have setup implicitly.
>
> I agree that we should keep flink-core as lean as possible.
>
> On guava usages in general my current stance is that we should only
> use guava if necessary. Spreading it (or other dependencies for that
> matter) to far just creates headaches when bumping the dependency.
>
> One core principle that we *must* follow is that shaded dependencies
> are neither exposed to user-code nor contained in the user-jar.
> Otherwise we end up again in a situation where we cannot increment
> dependencies without breaking compatibility for users.
>
> The PR at hand has a few issues in this regard. The guava limiter is
> only used in tests but is not a test class, yet isn't annotated with
> either Public(Evolving)/Internal. As it stands I cannot judge whether
> this is truly supposed to be an example / test utility or actually a
> user-facing class.
> If this is to be used by connectors, which are included in the
> user-jar, then we're violating the principle above, in which case the
> class should be relocated/removed.
>
> On 06.03.2019 15:10, Aljoscha Krettek wrote:
>> Hi,
>>
>> I recently saw that we added a dependency on our shaded-guava to
>> flink-core [1]. Just for the record, I don’t want do diminish the
>> contributions of anyone involved in the PR in any way. It just made
>> me realise that we have some implicit agreements or assumptions about
>> adding certain things to core packages that we might never have
>> really discussed. I think we should do that now.
>>
>> Quite some time ago an effort was started to reduce our dependency on
>> Guava [2] because of some problems with version stability and
>> dependency conflicts. At some later point, we created shaded guava so
>> that we could use it without clashes [3]. I believe we now have
>> shaded Guava only in runtime modules where it wasn’t easy to remove
>> and CEP.
>>
>> With the creation of shaded guava, I think we are in a bit of a limbo
>> situation where it is not exactly clear what our stance towards it
>> is, because it is easy to add to modules, as evident by the
>> aforementioned PR. I think we should discuss that situation and agree
>> upon a common stance on the topic.
>>
>> In general, I think the surface (which includes classes, interfaces,
>> and dependencies, among other things) of core modules should be kept
>> as lean as possible.  (all modules really)
>>
>> What do you think?
>>
>> Best,
>> Aljoscha
>>
>> [1] https://github.com/apache/flink/pull/7679 
>> <https://github.com/apache/flink/pull/7679>
>> [2] https://issues.apache.org/jira/browse/FLINK-3700 
>> <https://issues.apache.org/jira/browse/FLINK-3700>
>> [3] https://issues.apache.org/jira/browse/FLINK-6982
>

[Thomas Weise]

For more context, see [1] [2]

The GuavaFlinkConnectorRateLimiter is an implementation of the rate limiter
interface that uses Guava. It is not a test class, it is intended to be
used in applications and the (shaded) Guava isn't user facing.

[1]
https://lists.apache.org/thread.html/3599d95020604e2476bd794c55a11bc1c3a958a83a3c9c45c50630c1@%3Cdev.flink.apache.org%3E
[2]
https://lists.apache.org/thread.html/3599d95020604e2476bd794c55a11bc1c3a958a83a3c9c45c50630c1@%3Cdev.flink.apache.org%3E


On Wed, Mar 6, 2019 at 6:41 AM Chesnay Schepler <[hidden email]> wrote:

> I fully agree that we should write down this kind of conventions that
> committers have setup implicitly.
>
> I agree that we should keep flink-core as lean as possible.
>
> On guava usages in general my current stance is that we should only use
> guava if necessary. Spreading it (or other dependencies for that matter)
> to far just creates headaches when bumping the dependency.
>
> One core principle that we *must* follow is that shaded dependencies are
> neither exposed to user-code nor contained in the user-jar. Otherwise we
> end up again in a situation where we cannot increment dependencies
> without breaking compatibility for users.
>
> The PR at hand has a few issues in this regard. The guava limiter is
> only used in tests but is not a test class, yet isn't annotated with
> either Public(Evolving)/Internal. As it stands I cannot judge whether
> this is truly supposed to be an example / test utility or actually a
> user-facing class.
> If this is to be used by connectors, which are included in the user-jar,
> then we're violating the principle above, in which case the class should
> be relocated/removed.
>
> On 06.03.2019 15:10, Aljoscha Krettek wrote:
> > Hi,
> >
> > I recently saw that we added a dependency on our shaded-guava to
> flink-core [1]. Just for the record, I don’t want do diminish the
> contributions of anyone involved in the PR in any way. It just made me
> realise that we have some implicit agreements or assumptions about adding
> certain things to core packages that we might never have really discussed.
> I think we should do that now.
> >
> > Quite some time ago an effort was started to reduce our dependency on
> Guava [2] because of some problems with version stability and dependency
> conflicts. At some later point, we created shaded guava so that we could
> use it without clashes [3]. I believe we now have shaded Guava only in
> runtime modules where it wasn’t easy to remove and CEP.
> >
> > With the creation of shaded guava, I think we are in a bit of a limbo
> situation where it is not exactly clear what our stance towards it is,
> because it is easy to add to modules, as evident by the aforementioned PR.
> I think we should discuss that situation and agree upon a common stance on
> the topic.
> >
> > In general, I think the surface (which includes classes, interfaces, and
> dependencies, among other things) of core modules should be kept as lean as
> possible.  (all modules really)
> >
> > What do you think?
> >
> > Best,
> > Aljoscha
> >
> > [1] https://github.com/apache/flink/pull/7679 <
> https://github.com/apache/flink/pull/7679>
> > [2] https://issues.apache.org/jira/browse/FLINK-3700 <
> https://issues.apache.org/jira/browse/FLINK-3700>
> > [3] https://issues.apache.org/jira/browse/FLINK-6982
>
>
>

[Aljoscha Krettek-2]

I think the two links are identical.

> On 6. Mar 2019, at 16:05, Thomas Weise <[hidden email]> wrote:
>
> For more context, see [1] [2]
>
> The GuavaFlinkConnectorRateLimiter is an implementation of the rate limiter
> interface that uses Guava. It is not a test class, it is intended to be
> used in applications and the (shaded) Guava isn't user facing.
>
> [1]
> https://lists.apache.org/thread.html/3599d95020604e2476bd794c55a11bc1c3a958a83a3c9c45c50630c1@%3Cdev.flink.apache.org%3E
> [2]
> https://lists.apache.org/thread.html/3599d95020604e2476bd794c55a11bc1c3a958a83a3c9c45c50630c1@%3Cdev.flink.apache.org%3E
>
>
> On Wed, Mar 6, 2019 at 6:41 AM Chesnay Schepler <[hidden email]> wrote:
>
>> I fully agree that we should write down this kind of conventions that
>> committers have setup implicitly.
>>
>> I agree that we should keep flink-core as lean as possible.
>>
>> On guava usages in general my current stance is that we should only use
>> guava if necessary. Spreading it (or other dependencies for that matter)
>> to far just creates headaches when bumping the dependency.
>>
>> One core principle that we *must* follow is that shaded dependencies are
>> neither exposed to user-code nor contained in the user-jar. Otherwise we
>> end up again in a situation where we cannot increment dependencies
>> without breaking compatibility for users.
>>
>> The PR at hand has a few issues in this regard. The guava limiter is
>> only used in tests but is not a test class, yet isn't annotated with
>> either Public(Evolving)/Internal. As it stands I cannot judge whether
>> this is truly supposed to be an example / test utility or actually a
>> user-facing class.
>> If this is to be used by connectors, which are included in the user-jar,
>> then we're violating the principle above, in which case the class should
>> be relocated/removed.
>>
>> On 06.03.2019 15:10, Aljoscha Krettek wrote:
>>> Hi,
>>>
>>> I recently saw that we added a dependency on our shaded-guava to
>> flink-core [1]. Just for the record, I don’t want do diminish the
>> contributions of anyone involved in the PR in any way. It just made me
>> realise that we have some implicit agreements or assumptions about adding
>> certain things to core packages that we might never have really discussed.
>> I think we should do that now.
>>>
>>> Quite some time ago an effort was started to reduce our dependency on
>> Guava [2] because of some problems with version stability and dependency
>> conflicts. At some later point, we created shaded guava so that we could
>> use it without clashes [3]. I believe we now have shaded Guava only in
>> runtime modules where it wasn’t easy to remove and CEP.
>>>
>>> With the creation of shaded guava, I think we are in a bit of a limbo
>> situation where it is not exactly clear what our stance towards it is,
>> because it is easy to add to modules, as evident by the aforementioned PR.
>> I think we should discuss that situation and agree upon a common stance on
>> the topic.
>>>
>>> In general, I think the surface (which includes classes, interfaces, and
>> dependencies, among other things) of core modules should be kept as lean as
>> possible.  (all modules really)
>>>
>>> What do you think?
>>>
>>> Best,
>>> Aljoscha
>>>
>>> [1] https://github.com/apache/flink/pull/7679 <
>> https://github.com/apache/flink/pull/7679>
>>> [2] https://issues.apache.org/jira/browse/FLINK-3700 <
>> https://issues.apache.org/jira/browse/FLINK-3700>
>>> [3] https://issues.apache.org/jira/browse/FLINK-6982
>>
>>
>>

[Thomas Weise]

How I managed to do that..

Here is the discussion about the shared package:

https://lists.apache.org/thread.html/3de9d2353cf22aea0448fb744314103b5f88195216acc3bff449354a@%3Cdev.flink.apache.org%3E


On Wed, Mar 6, 2019 at 7:10 AM Aljoscha Krettek <[hidden email]> wrote:

> I think the two links are identical.
>
> > On 6. Mar 2019, at 16:05, Thomas Weise <[hidden email]> wrote:
> >
> > For more context, see [1] [2]
> >
> > The GuavaFlinkConnectorRateLimiter is an implementation of the rate
> limiter
> > interface that uses Guava. It is not a test class, it is intended to be
> > used in applications and the (shaded) Guava isn't user facing.
> >
> > [1]
> >
> https://lists.apache.org/thread.html/3599d95020604e2476bd794c55a11bc1c3a958a83a3c9c45c50630c1@%3Cdev.flink.apache.org%3E
> > [2]
> >
> https://lists.apache.org/thread.html/3599d95020604e2476bd794c55a11bc1c3a958a83a3c9c45c50630c1@%3Cdev.flink.apache.org%3E
> >
> >
> > On Wed, Mar 6, 2019 at 6:41 AM Chesnay Schepler <[hidden email]>
> wrote:
> >
> >> I fully agree that we should write down this kind of conventions that
> >> committers have setup implicitly.
> >>
> >> I agree that we should keep flink-core as lean as possible.
> >>
> >> On guava usages in general my current stance is that we should only use
> >> guava if necessary. Spreading it (or other dependencies for that matter)
> >> to far just creates headaches when bumping the dependency.
> >>
> >> One core principle that we *must* follow is that shaded dependencies are
> >> neither exposed to user-code nor contained in the user-jar. Otherwise we
> >> end up again in a situation where we cannot increment dependencies
> >> without breaking compatibility for users.
> >>
> >> The PR at hand has a few issues in this regard. The guava limiter is
> >> only used in tests but is not a test class, yet isn't annotated with
> >> either Public(Evolving)/Internal. As it stands I cannot judge whether
> >> this is truly supposed to be an example / test utility or actually a
> >> user-facing class.
> >> If this is to be used by connectors, which are included in the user-jar,
> >> then we're violating the principle above, in which case the class should
> >> be relocated/removed.
> >>
> >> On 06.03.2019 15:10, Aljoscha Krettek wrote:
> >>> Hi,
> >>>
> >>> I recently saw that we added a dependency on our shaded-guava to
> >> flink-core [1]. Just for the record, I don’t want do diminish the
> >> contributions of anyone involved in the PR in any way. It just made me
> >> realise that we have some implicit agreements or assumptions about
> adding
> >> certain things to core packages that we might never have really
> discussed.
> >> I think we should do that now.
> >>>
> >>> Quite some time ago an effort was started to reduce our dependency on
> >> Guava [2] because of some problems with version stability and dependency
> >> conflicts. At some later point, we created shaded guava so that we could
> >> use it without clashes [3]. I believe we now have shaded Guava only in
> >> runtime modules where it wasn’t easy to remove and CEP.
> >>>
> >>> With the creation of shaded guava, I think we are in a bit of a limbo
> >> situation where it is not exactly clear what our stance towards it is,
> >> because it is easy to add to modules, as evident by the aforementioned
> PR.
> >> I think we should discuss that situation and agree upon a common stance
> on
> >> the topic.
> >>>
> >>> In general, I think the surface (which includes classes, interfaces,
> and
> >> dependencies, among other things) of core modules should be kept as
> lean as
> >> possible.  (all modules really)
> >>>
> >>> What do you think?
> >>>
> >>> Best,
> >>> Aljoscha
> >>>
> >>> [1] https://github.com/apache/flink/pull/7679 <
> >> https://github.com/apache/flink/pull/7679>
> >>> [2] https://issues.apache.org/jira/browse/FLINK-3700 <
> >> https://issues.apache.org/jira/browse/FLINK-3700>
> >>> [3] https://issues.apache.org/jira/browse/FLINK-6982
> >>
> >>
> >>
>
>

[Thomas Weise]

I want to bring this back to attention - with the concern raised here we
are still looking for a potentially better answer to the original issue [1].

We have a class that depends on Guava (in its implementation, not the
public interface) and we want to make it available for use with multiple
connectors.

Thanks,
Thomas

[1]
https://lists.apache.org/thread.html/3de9d2353cf22aea0448fb744314103b5f88195216acc3bff449354a@%3Cdev.flink.apache.org%3E

On Wed, Mar 6, 2019 at 7:13 AM Thomas Weise <[hidden email]> wrote:

> How I managed to do that..
>
> Here is the discussion about the shared package:
>
>
> https://lists.apache.org/thread.html/3de9d2353cf22aea0448fb744314103b5f88195216acc3bff449354a@%3Cdev.flink.apache.org%3E
>
>
> On Wed, Mar 6, 2019 at 7:10 AM Aljoscha Krettek <[hidden email]>
> wrote:
>
>> I think the two links are identical.
>>
>> > On 6. Mar 2019, at 16:05, Thomas Weise <[hidden email]> wrote:
>> >
>> > For more context, see [1] [2]
>> >
>> > The GuavaFlinkConnectorRateLimiter is an implementation of the rate
>> limiter
>> > interface that uses Guava. It is not a test class, it is intended to be
>> > used in applications and the (shaded) Guava isn't user facing.
>> >
>> > [1]
>> >
>> https://lists.apache.org/thread.html/3599d95020604e2476bd794c55a11bc1c3a958a83a3c9c45c50630c1@%3Cdev.flink.apache.org%3E
>> > [2]
>> >
>> https://lists.apache.org/thread.html/3599d95020604e2476bd794c55a11bc1c3a958a83a3c9c45c50630c1@%3Cdev.flink.apache.org%3E
>> >
>> >
>> > On Wed, Mar 6, 2019 at 6:41 AM Chesnay Schepler <[hidden email]>
>> wrote:
>> >
>> >> I fully agree that we should write down this kind of conventions that
>> >> committers have setup implicitly.
>> >>
>> >> I agree that we should keep flink-core as lean as possible.
>> >>
>> >> On guava usages in general my current stance is that we should only use
>> >> guava if necessary. Spreading it (or other dependencies for that
>> matter)
>> >> to far just creates headaches when bumping the dependency.
>> >>
>> >> One core principle that we *must* follow is that shaded dependencies
>> are
>> >> neither exposed to user-code nor contained in the user-jar. Otherwise
>> we
>> >> end up again in a situation where we cannot increment dependencies
>> >> without breaking compatibility for users.
>> >>
>> >> The PR at hand has a few issues in this regard. The guava limiter is
>> >> only used in tests but is not a test class, yet isn't annotated with
>> >> either Public(Evolving)/Internal. As it stands I cannot judge whether
>> >> this is truly supposed to be an example / test utility or actually a
>> >> user-facing class.
>> >> If this is to be used by connectors, which are included in the
>> user-jar,
>> >> then we're violating the principle above, in which case the class
>> should
>> >> be relocated/removed.
>> >>
>> >> On 06.03.2019 15:10, Aljoscha Krettek wrote:
>> >>> Hi,
>> >>>
>> >>> I recently saw that we added a dependency on our shaded-guava to
>> >> flink-core [1]. Just for the record, I don’t want do diminish the
>> >> contributions of anyone involved in the PR in any way. It just made me
>> >> realise that we have some implicit agreements or assumptions about
>> adding
>> >> certain things to core packages that we might never have really
>> discussed.
>> >> I think we should do that now.
>> >>>
>> >>> Quite some time ago an effort was started to reduce our dependency on
>> >> Guava [2] because of some problems with version stability and
>> dependency
>> >> conflicts. At some later point, we created shaded guava so that we
>> could
>> >> use it without clashes [3]. I believe we now have shaded Guava only in
>> >> runtime modules where it wasn’t easy to remove and CEP.
>> >>>
>> >>> With the creation of shaded guava, I think we are in a bit of a limbo
>> >> situation where it is not exactly clear what our stance towards it is,
>> >> because it is easy to add to modules, as evident by the aforementioned
>> PR.
>> >> I think we should discuss that situation and agree upon a common
>> stance on
>> >> the topic.
>> >>>
>> >>> In general, I think the surface (which includes classes, interfaces,
>> and
>> >> dependencies, among other things) of core modules should be kept as
>> lean as
>> >> possible.  (all modules really)
>> >>>
>> >>> What do you think?
>> >>>
>> >>> Best,
>> >>> Aljoscha
>> >>>
>> >>> [1] https://github.com/apache/flink/pull/7679 <
>> >> https://github.com/apache/flink/pull/7679>
>> >>> [2] https://issues.apache.org/jira/browse/FLINK-3700 <
>> >> https://issues.apache.org/jira/browse/FLINK-3700>
>> >>> [3] https://issues.apache.org/jira/browse/FLINK-6982
>> >>
>> >>
>> >>
>>
>>

[Aljoscha Krettek-2]

Hi,

By now, I think it’s fine to have Guava (our shaded guava) as a dependency in flink-core. It’s a very useful library and I don’t see problems with version clashes because it’s relocated to our namespace.

If no-one objects, I would just leave the dependency in as-is and conclude this discussion.

Best,
Aljoscha

> On 13. Mar 2019, at 05:10, Thomas Weise <[hidden email]> wrote:
>
> I want to bring this back to attention - with the concern raised here we
> are still looking for a potentially better answer to the original issue [1].
>
> We have a class that depends on Guava (in its implementation, not the
> public interface) and we want to make it available for use with multiple
> connectors.
>
> Thanks,
> Thomas
>
> [1]
> https://lists.apache.org/thread.html/3de9d2353cf22aea0448fb744314103b5f88195216acc3bff449354a@%3Cdev.flink.apache.org%3E
>
> On Wed, Mar 6, 2019 at 7:13 AM Thomas Weise <[hidden email]> wrote:
>
>> How I managed to do that..
>>
>> Here is the discussion about the shared package:
>>
>>
>> https://lists.apache.org/thread.html/3de9d2353cf22aea0448fb744314103b5f88195216acc3bff449354a@%3Cdev.flink.apache.org%3E
>>
>>
>> On Wed, Mar 6, 2019 at 7:10 AM Aljoscha Krettek <[hidden email]>
>> wrote:
>>
>>> I think the two links are identical.
>>>
>>>> On 6. Mar 2019, at 16:05, Thomas Weise <[hidden email]> wrote:
>>>>
>>>> For more context, see [1] [2]
>>>>
>>>> The GuavaFlinkConnectorRateLimiter is an implementation of the rate
>>> limiter
>>>> interface that uses Guava. It is not a test class, it is intended to be
>>>> used in applications and the (shaded) Guava isn't user facing.
>>>>
>>>> [1]
>>>>
>>> https://lists.apache.org/thread.html/3599d95020604e2476bd794c55a11bc1c3a958a83a3c9c45c50630c1@%3Cdev.flink.apache.org%3E
>>>> [2]
>>>>
>>> https://lists.apache.org/thread.html/3599d95020604e2476bd794c55a11bc1c3a958a83a3c9c45c50630c1@%3Cdev.flink.apache.org%3E
>>>>
>>>>
>>>> On Wed, Mar 6, 2019 at 6:41 AM Chesnay Schepler <[hidden email]>
>>> wrote:
>>>>
>>>>> I fully agree that we should write down this kind of conventions that
>>>>> committers have setup implicitly.
>>>>>
>>>>> I agree that we should keep flink-core as lean as possible.
>>>>>
>>>>> On guava usages in general my current stance is that we should only use
>>>>> guava if necessary. Spreading it (or other dependencies for that
>>> matter)
>>>>> to far just creates headaches when bumping the dependency.
>>>>>
>>>>> One core principle that we *must* follow is that shaded dependencies
>>> are
>>>>> neither exposed to user-code nor contained in the user-jar. Otherwise
>>> we
>>>>> end up again in a situation where we cannot increment dependencies
>>>>> without breaking compatibility for users.
>>>>>
>>>>> The PR at hand has a few issues in this regard. The guava limiter is
>>>>> only used in tests but is not a test class, yet isn't annotated with
>>>>> either Public(Evolving)/Internal. As it stands I cannot judge whether
>>>>> this is truly supposed to be an example / test utility or actually a
>>>>> user-facing class.
>>>>> If this is to be used by connectors, which are included in the
>>> user-jar,
>>>>> then we're violating the principle above, in which case the class
>>> should
>>>>> be relocated/removed.
>>>>>
>>>>> On 06.03.2019 15:10, Aljoscha Krettek wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I recently saw that we added a dependency on our shaded-guava to
>>>>> flink-core [1]. Just for the record, I don’t want do diminish the
>>>>> contributions of anyone involved in the PR in any way. It just made me
>>>>> realise that we have some implicit agreements or assumptions about
>>> adding
>>>>> certain things to core packages that we might never have really
>>> discussed.
>>>>> I think we should do that now.
>>>>>>
>>>>>> Quite some time ago an effort was started to reduce our dependency on
>>>>> Guava [2] because of some problems with version stability and
>>> dependency
>>>>> conflicts. At some later point, we created shaded guava so that we
>>> could
>>>>> use it without clashes [3]. I believe we now have shaded Guava only in
>>>>> runtime modules where it wasn’t easy to remove and CEP.
>>>>>>
>>>>>> With the creation of shaded guava, I think we are in a bit of a limbo
>>>>> situation where it is not exactly clear what our stance towards it is,
>>>>> because it is easy to add to modules, as evident by the aforementioned
>>> PR.
>>>>> I think we should discuss that situation and agree upon a common
>>> stance on
>>>>> the topic.
>>>>>>
>>>>>> In general, I think the surface (which includes classes, interfaces,
>>> and
>>>>> dependencies, among other things) of core modules should be kept as
>>> lean as
>>>>> possible.  (all modules really)
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>>> Best,
>>>>>> Aljoscha
>>>>>>
>>>>>> [1] https://github.com/apache/flink/pull/7679 <
>>>>> https://github.com/apache/flink/pull/7679>
>>>>>> [2] https://issues.apache.org/jira/browse/FLINK-3700 <
>>>>> https://issues.apache.org/jira/browse/FLINK-3700>
>>>>>> [3] https://issues.apache.org/jira/browse/FLINK-6982
>>>>>
>>>>>
>>>>>
>>>
>>>

[Stephan Ewen]

I would be fine with that as well.

Let's keep the attitude to maintain a slim dependency footprint, but Guava
could be part of that small footprint.
As long as we ensure it is our vendored (pre-shaded) version and all
modules use the same version (managed version in the root pom).

Best,
Stephan


On Mon, Mar 25, 2019 at 10:34 AM Aljoscha Krettek <[hidden email]>
wrote:

> Hi,
>
> By now, I think it’s fine to have Guava (our shaded guava) as a dependency
> in flink-core. It’s a very useful library and I don’t see problems with
> version clashes because it’s relocated to our namespace.
>
> If no-one objects, I would just leave the dependency in as-is and conclude
> this discussion.
>
> Best,
> Aljoscha
>
> > On 13. Mar 2019, at 05:10, Thomas Weise <[hidden email]> wrote:
> >
> > I want to bring this back to attention - with the concern raised here we
> > are still looking for a potentially better answer to the original issue
> [1].
> >
> > We have a class that depends on Guava (in its implementation, not the
> > public interface) and we want to make it available for use with multiple
> > connectors.
> >
> > Thanks,
> > Thomas
> >
> > [1]
> >
> https://lists.apache.org/thread.html/3de9d2353cf22aea0448fb744314103b5f88195216acc3bff449354a@%3Cdev.flink.apache.org%3E
> >
> > On Wed, Mar 6, 2019 at 7:13 AM Thomas Weise <[hidden email]> wrote:
> >
> >> How I managed to do that..
> >>
> >> Here is the discussion about the shared package:
> >>
> >>
> >>
> https://lists.apache.org/thread.html/3de9d2353cf22aea0448fb744314103b5f88195216acc3bff449354a@%3Cdev.flink.apache.org%3E
> >>
> >>
> >> On Wed, Mar 6, 2019 at 7:10 AM Aljoscha Krettek <[hidden email]>
> >> wrote:
> >>
> >>> I think the two links are identical.
> >>>
> >>>> On 6. Mar 2019, at 16:05, Thomas Weise <[hidden email]> wrote:
> >>>>
> >>>> For more context, see [1] [2]
> >>>>
> >>>> The GuavaFlinkConnectorRateLimiter is an implementation of the rate
> >>> limiter
> >>>> interface that uses Guava. It is not a test class, it is intended to
> be
> >>>> used in applications and the (shaded) Guava isn't user facing.
> >>>>
> >>>> [1]
> >>>>
> >>>
> https://lists.apache.org/thread.html/3599d95020604e2476bd794c55a11bc1c3a958a83a3c9c45c50630c1@%3Cdev.flink.apache.org%3E
> >>>> [2]
> >>>>
> >>>
> https://lists.apache.org/thread.html/3599d95020604e2476bd794c55a11bc1c3a958a83a3c9c45c50630c1@%3Cdev.flink.apache.org%3E
> >>>>
> >>>>
> >>>> On Wed, Mar 6, 2019 at 6:41 AM Chesnay Schepler <[hidden email]>
> >>> wrote:
> >>>>
> >>>>> I fully agree that we should write down this kind of conventions that
> >>>>> committers have setup implicitly.
> >>>>>
> >>>>> I agree that we should keep flink-core as lean as possible.
> >>>>>
> >>>>> On guava usages in general my current stance is that we should only
> use
> >>>>> guava if necessary. Spreading it (or other dependencies for that
> >>> matter)
> >>>>> to far just creates headaches when bumping the dependency.
> >>>>>
> >>>>> One core principle that we *must* follow is that shaded dependencies
> >>> are
> >>>>> neither exposed to user-code nor contained in the user-jar. Otherwise
> >>> we
> >>>>> end up again in a situation where we cannot increment dependencies
> >>>>> without breaking compatibility for users.
> >>>>>
> >>>>> The PR at hand has a few issues in this regard. The guava limiter is
> >>>>> only used in tests but is not a test class, yet isn't annotated with
> >>>>> either Public(Evolving)/Internal. As it stands I cannot judge whether
> >>>>> this is truly supposed to be an example / test utility or actually a
> >>>>> user-facing class.
> >>>>> If this is to be used by connectors, which are included in the
> >>> user-jar,
> >>>>> then we're violating the principle above, in which case the class
> >>> should
> >>>>> be relocated/removed.
> >>>>>
> >>>>> On 06.03.2019 15:10, Aljoscha Krettek wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> I recently saw that we added a dependency on our shaded-guava to
> >>>>> flink-core [1]. Just for the record, I don’t want do diminish the
> >>>>> contributions of anyone involved in the PR in any way. It just made
> me
> >>>>> realise that we have some implicit agreements or assumptions about
> >>> adding
> >>>>> certain things to core packages that we might never have really
> >>> discussed.
> >>>>> I think we should do that now.
> >>>>>>
> >>>>>> Quite some time ago an effort was started to reduce our dependency
> on
> >>>>> Guava [2] because of some problems with version stability and
> >>> dependency
> >>>>> conflicts. At some later point, we created shaded guava so that we
> >>> could
> >>>>> use it without clashes [3]. I believe we now have shaded Guava only
> in
> >>>>> runtime modules where it wasn’t easy to remove and CEP.
> >>>>>>
> >>>>>> With the creation of shaded guava, I think we are in a bit of a
> limbo
> >>>>> situation where it is not exactly clear what our stance towards it
> is,
> >>>>> because it is easy to add to modules, as evident by the
> aforementioned
> >>> PR.
> >>>>> I think we should discuss that situation and agree upon a common
> >>> stance on
> >>>>> the topic.
> >>>>>>
> >>>>>> In general, I think the surface (which includes classes, interfaces,
> >>> and
> >>>>> dependencies, among other things) of core modules should be kept as
> >>> lean as
> >>>>> possible.  (all modules really)
> >>>>>>
> >>>>>> What do you think?
> >>>>>>
> >>>>>> Best,
> >>>>>> Aljoscha
> >>>>>>
> >>>>>> [1] https://github.com/apache/flink/pull/7679 <
> >>>>> https://github.com/apache/flink/pull/7679>
> >>>>>> [2] https://issues.apache.org/jira/browse/FLINK-3700 <
> >>>>> https://issues.apache.org/jira/browse/FLINK-3700>
> >>>>>> [3] https://issues.apache.org/jira/browse/FLINK-6982
> >>>>>
> >>>>>
> >>>>>
> >>>
> >>>
>
>

[Thomas Weise]

Hi,

Thanks for the follow-up.

@Aljoscha Krettek <[hidden email]> if there is consensus, can we also
backport FLINK-11501 for 1.8.1? (The Guava dependency was the reason we
didn't.)

Thanks,
Thomas


On Mon, Mar 25, 2019 at 8:07 AM Stephan Ewen <[hidden email]> wrote:

> I would be fine with that as well.
>
> Let's keep the attitude to maintain a slim dependency footprint, but Guava
> could be part of that small footprint.
> As long as we ensure it is our vendored (pre-shaded) version and all
> modules use the same version (managed version in the root pom).
>
> Best,
> Stephan
>
>
> On Mon, Mar 25, 2019 at 10:34 AM Aljoscha Krettek <[hidden email]>
> wrote:
>
> > Hi,
> >
> > By now, I think it’s fine to have Guava (our shaded guava) as a
> dependency
> > in flink-core. It’s a very useful library and I don’t see problems with
> > version clashes because it’s relocated to our namespace.
> >
> > If no-one objects, I would just leave the dependency in as-is and
> conclude
> > this discussion.
> >
> > Best,
> > Aljoscha
> >
> > > On 13. Mar 2019, at 05:10, Thomas Weise <[hidden email]> wrote:
> > >
> > > I want to bring this back to attention - with the concern raised here
> we
> > > are still looking for a potentially better answer to the original issue
> > [1].
> > >
> > > We have a class that depends on Guava (in its implementation, not the
> > > public interface) and we want to make it available for use with
> multiple
> > > connectors.
> > >
> > > Thanks,
> > > Thomas
> > >
> > > [1]
> > >
> >
> https://lists.apache.org/thread.html/3de9d2353cf22aea0448fb744314103b5f88195216acc3bff449354a@%3Cdev.flink.apache.org%3E
> > >
> > > On Wed, Mar 6, 2019 at 7:13 AM Thomas Weise <[hidden email]> wrote:
> > >
> > >> How I managed to do that..
> > >>
> > >> Here is the discussion about the shared package:
> > >>
> > >>
> > >>
> >
> https://lists.apache.org/thread.html/3de9d2353cf22aea0448fb744314103b5f88195216acc3bff449354a@%3Cdev.flink.apache.org%3E
> > >>
> > >>
> > >> On Wed, Mar 6, 2019 at 7:10 AM Aljoscha Krettek <[hidden email]>
> > >> wrote:
> > >>
> > >>> I think the two links are identical.
> > >>>
> > >>>> On 6. Mar 2019, at 16:05, Thomas Weise <[hidden email]> wrote:
> > >>>>
> > >>>> For more context, see [1] [2]
> > >>>>
> > >>>> The GuavaFlinkConnectorRateLimiter is an implementation of the rate
> > >>> limiter
> > >>>> interface that uses Guava. It is not a test class, it is intended to
> > be
> > >>>> used in applications and the (shaded) Guava isn't user facing.
> > >>>>
> > >>>> [1]
> > >>>>
> > >>>
> >
> https://lists.apache.org/thread.html/3599d95020604e2476bd794c55a11bc1c3a958a83a3c9c45c50630c1@%3Cdev.flink.apache.org%3E
> > >>>> [2]
> > >>>>
> > >>>
> >
> https://lists.apache.org/thread.html/3599d95020604e2476bd794c55a11bc1c3a958a83a3c9c45c50630c1@%3Cdev.flink.apache.org%3E
> > >>>>
> > >>>>
> > >>>> On Wed, Mar 6, 2019 at 6:41 AM Chesnay Schepler <[hidden email]
> >
> > >>> wrote:
> > >>>>
> > >>>>> I fully agree that we should write down this kind of conventions
> that
> > >>>>> committers have setup implicitly.
> > >>>>>
> > >>>>> I agree that we should keep flink-core as lean as possible.
> > >>>>>
> > >>>>> On guava usages in general my current stance is that we should only
> > use
> > >>>>> guava if necessary. Spreading it (or other dependencies for that
> > >>> matter)
> > >>>>> to far just creates headaches when bumping the dependency.
> > >>>>>
> > >>>>> One core principle that we *must* follow is that shaded
> dependencies
> > >>> are
> > >>>>> neither exposed to user-code nor contained in the user-jar.
> Otherwise
> > >>> we
> > >>>>> end up again in a situation where we cannot increment dependencies
> > >>>>> without breaking compatibility for users.
> > >>>>>
> > >>>>> The PR at hand has a few issues in this regard. The guava limiter
> is
> > >>>>> only used in tests but is not a test class, yet isn't annotated
> with
> > >>>>> either Public(Evolving)/Internal. As it stands I cannot judge
> whether
> > >>>>> this is truly supposed to be an example / test utility or actually
> a
> > >>>>> user-facing class.
> > >>>>> If this is to be used by connectors, which are included in the
> > >>> user-jar,
> > >>>>> then we're violating the principle above, in which case the class
> > >>> should
> > >>>>> be relocated/removed.
> > >>>>>
> > >>>>> On 06.03.2019 15:10, Aljoscha Krettek wrote:
> > >>>>>> Hi,
> > >>>>>>
> > >>>>>> I recently saw that we added a dependency on our shaded-guava to
> > >>>>> flink-core [1]. Just for the record, I don’t want do diminish the
> > >>>>> contributions of anyone involved in the PR in any way. It just made
> > me
> > >>>>> realise that we have some implicit agreements or assumptions about
> > >>> adding
> > >>>>> certain things to core packages that we might never have really
> > >>> discussed.
> > >>>>> I think we should do that now.
> > >>>>>>
> > >>>>>> Quite some time ago an effort was started to reduce our dependency
> > on
> > >>>>> Guava [2] because of some problems with version stability and
> > >>> dependency
> > >>>>> conflicts. At some later point, we created shaded guava so that we
> > >>> could
> > >>>>> use it without clashes [3]. I believe we now have shaded Guava only
> > in
> > >>>>> runtime modules where it wasn’t easy to remove and CEP.
> > >>>>>>
> > >>>>>> With the creation of shaded guava, I think we are in a bit of a
> > limbo
> > >>>>> situation where it is not exactly clear what our stance towards it
> > is,
> > >>>>> because it is easy to add to modules, as evident by the
> > aforementioned
> > >>> PR.
> > >>>>> I think we should discuss that situation and agree upon a common
> > >>> stance on
> > >>>>> the topic.
> > >>>>>>
> > >>>>>> In general, I think the surface (which includes classes,
> interfaces,
> > >>> and
> > >>>>> dependencies, among other things) of core modules should be kept as
> > >>> lean as
> > >>>>> possible.  (all modules really)
> > >>>>>>
> > >>>>>> What do you think?
> > >>>>>>
> > >>>>>> Best,
> > >>>>>> Aljoscha
> > >>>>>>
> > >>>>>> [1] https://github.com/apache/flink/pull/7679 <
> > >>>>> https://github.com/apache/flink/pull/7679>
> > >>>>>> [2] https://issues.apache.org/jira/browse/FLINK-3700 <
> > >>>>> https://issues.apache.org/jira/browse/FLINK-3700>
> > >>>>>> [3] https://issues.apache.org/jira/browse/FLINK-6982
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>
> > >>>
> >
> >
>