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
