[DISCUSS] Consolidated log4j2-properties file

[Chesnay Schepler-3]

Hello,

I discovered a handy trick that would allow us to share a single
log4j2-test.properties across all modules.

https://github.com/apache/flink/pull/11634

The file would exist in flink-test-utils-junit/src/main/resources, and
be used for all modules except the kafka connectors and yarn-tests
(because they have some custom requirements).

This would mean the files can no longer go out of sync, utilities can be
shared more easily, and you wouldn't need to add a new properties file
to new modules (or older ones lacking one) during debugging.

Overall I personally quite, but I have heard some concerns about
changing dev routines so I wanted to double-check what people think in
general.

[Till Rohrmann]

Hi Chesnay,

thanks for kicking this discussion off. I agree that deduplicating code is
in general a good idea.

The main benefit seems to be that all modules inherit a
log4j2-test.properties file and that this file allows to control the
logging output for several modules.

The main drawback I see is that it complicates the debugging process for
our devs. If you want to debug a problem and need logging for this, then
you have to know that there is a log4j2-test.properties file in
flink-test-utils-junit which you can tweak. At the moment, it is quite
straight forward as you simply go to the module which contains the test and
check the resources folder.

If we are ok with this drawback and document the change properly, then I'm
fine with this change.

Cheers,
Till

On Mon, Apr 6, 2020 at 12:24 PM Chesnay Schepler <[hidden email]> wrote:

> Hello,
>
> I discovered a handy trick that would allow us to share a single
> log4j2-test.properties across all modules.
>
> https://github.com/apache/flink/pull/11634
>
> The file would exist in flink-test-utils-junit/src/main/resources, and
> be used for all modules except the kafka connectors and yarn-tests
> (because they have some custom requirements).
>
> This would mean the files can no longer go out of sync, utilities can be
> shared more easily, and you wouldn't need to add a new properties file
> to new modules (or older ones lacking one) during debugging.
>
> Overall I personally quite, but I have heard some concerns about
> changing dev routines so I wanted to double-check what people think in
> general.
>
>

[Chesnay Schepler-3]

We can also move the file to the root of the project, which should make
it easier to discover.

flink-test-utils-junit would then just be a distribution vehicle that
few would have to know about.

On 06/04/2020 13:31, Till Rohrmann wrote:

> Hi Chesnay,
>
> thanks for kicking this discussion off. I agree that deduplicating code is
> in general a good idea.
>
> The main benefit seems to be that all modules inherit a
> log4j2-test.properties file and that this file allows to control the
> logging output for several modules.
>
> The main drawback I see is that it complicates the debugging process for
> our devs. If you want to debug a problem and need logging for this, then
> you have to know that there is a log4j2-test.properties file in
> flink-test-utils-junit which you can tweak. At the moment, it is quite
> straight forward as you simply go to the module which contains the test and
> check the resources folder.
>
> If we are ok with this drawback and document the change properly, then I'm
> fine with this change.
>
> Cheers,
> Till
>
> On Mon, Apr 6, 2020 at 12:24 PM Chesnay Schepler <[hidden email]> wrote:
>
>> Hello,
>>
>> I discovered a handy trick that would allow us to share a single
>> log4j2-test.properties across all modules.
>>
>> https://github.com/apache/flink/pull/11634
>>
>> The file would exist in flink-test-utils-junit/src/main/resources, and
>> be used for all modules except the kafka connectors and yarn-tests
>> (because they have some custom requirements).
>>
>> This would mean the files can no longer go out of sync, utilities can be
>> shared more easily, and you wouldn't need to add a new properties file
>> to new modules (or older ones lacking one) during debugging.
>>
>> Overall I personally quite, but I have heard some concerns about
>> changing dev routines so I wanted to double-check what people think in
>> general.
>>
>>

[Chesnay Schepler-3]

Actually, I would first have to double-check whether this would work
within IntelliJ...

On 06/04/2020 20:40, Chesnay Schepler wrote:

> We can also move the file to the root of the project, which should
> make it easier to discover.
>
> flink-test-utils-junit would then just be a distribution vehicle that
> few would have to know about.
>
> On 06/04/2020 13:31, Till Rohrmann wrote:
>> Hi Chesnay,
>>
>> thanks for kicking this discussion off. I agree that deduplicating
>> code is
>> in general a good idea.
>>
>> The main benefit seems to be that all modules inherit a
>> log4j2-test.properties file and that this file allows to control the
>> logging output for several modules.
>>
>> The main drawback I see is that it complicates the debugging process for
>> our devs. If you want to debug a problem and need logging for this, then
>> you have to know that there is a log4j2-test.properties file in
>> flink-test-utils-junit which you can tweak. At the moment, it is quite
>> straight forward as you simply go to the module which contains the
>> test and
>> check the resources folder.
>>
>> If we are ok with this drawback and document the change properly,
>> then I'm
>> fine with this change.
>>
>> Cheers,
>> Till
>>
>> On Mon, Apr 6, 2020 at 12:24 PM Chesnay Schepler <[hidden email]>
>> wrote:
>>
>>> Hello,
>>>
>>> I discovered a handy trick that would allow us to share a single
>>> log4j2-test.properties across all modules.
>>>
>>> https://github.com/apache/flink/pull/11634
>>>
>>> The file would exist in flink-test-utils-junit/src/main/resources, and
>>> be used for all modules except the kafka connectors and yarn-tests
>>> (because they have some custom requirements).
>>>
>>> This would mean the files can no longer go out of sync, utilities
>>> can be
>>> shared more easily, and you wouldn't need to add a new properties file
>>> to new modules (or older ones lacking one) during debugging.
>>>
>>> Overall I personally quite, but I have heard some concerns about
>>> changing dev routines so I wanted to double-check what people think in
>>> general.
>>>
>>>
>
>

[Chesnay Schepler-3]

Nope, it wouldn't work to have it in the root of the project; in a
practical sense IntelliJ can only really handle them in the standard
location.

Ironically, the PR currently only works in IntelliJ, because when you
build the jar with maven we exclude all log4j files via the
shade-plugin. Which we could of course fix, but we'd need another module
to override the shade-plugin in to avoid side-effects on
flink-test-utils-junit.
I suppose one positive thing would be that we could name the module
"flink-log4j2-test-configuration" which would at least be a more obvious
location...

On 06/04/2020 20:53, Chesnay Schepler wrote:

> Actually, I would first have to double-check whether this would work
> within IntelliJ...
>
> On 06/04/2020 20:40, Chesnay Schepler wrote:
>> We can also move the file to the root of the project, which should
>> make it easier to discover.
>>
>> flink-test-utils-junit would then just be a distribution vehicle that
>> few would have to know about.
>>
>> On 06/04/2020 13:31, Till Rohrmann wrote:
>>> Hi Chesnay,
>>>
>>> thanks for kicking this discussion off. I agree that deduplicating
>>> code is
>>> in general a good idea.
>>>
>>> The main benefit seems to be that all modules inherit a
>>> log4j2-test.properties file and that this file allows to control the
>>> logging output for several modules.
>>>
>>> The main drawback I see is that it complicates the debugging process
>>> for
>>> our devs. If you want to debug a problem and need logging for this,
>>> then
>>> you have to know that there is a log4j2-test.properties file in
>>> flink-test-utils-junit which you can tweak. At the moment, it is quite
>>> straight forward as you simply go to the module which contains the
>>> test and
>>> check the resources folder.
>>>
>>> If we are ok with this drawback and document the change properly,
>>> then I'm
>>> fine with this change.
>>>
>>> Cheers,
>>> Till
>>>
>>> On Mon, Apr 6, 2020 at 12:24 PM Chesnay Schepler
>>> <[hidden email]> wrote:
>>>
>>>> Hello,
>>>>
>>>> I discovered a handy trick that would allow us to share a single
>>>> log4j2-test.properties across all modules.
>>>>
>>>> https://github.com/apache/flink/pull/11634
>>>>
>>>> The file would exist in flink-test-utils-junit/src/main/resources, and
>>>> be used for all modules except the kafka connectors and yarn-tests
>>>> (because they have some custom requirements).
>>>>
>>>> This would mean the files can no longer go out of sync, utilities
>>>> can be
>>>> shared more easily, and you wouldn't need to add a new properties file
>>>> to new modules (or older ones lacking one) during debugging.
>>>>
>>>> Overall I personally quite, but I have heard some concerns about
>>>> changing dev routines so I wanted to double-check what people think in
>>>> general.
>>>>
>>>>
>>
>>
>
>

[Robert Metzger]

I also like the idea of getting rid of all those copies of the same file
across our codebase.

How about setting the log level in the log4j config file in
flink-test-utils-junit/src/main/resources to INFO, and using a separate
log4j config for local maven runs? (we pass a different log4j file in the
surefire config (not sure if that really works)).

This way, we don't pollute the command line when building Flink locally
through maven and developers get INFO log output when running tests from
the IDE.
This would reduce the burden of locating and changing the file every time
one looks into a test.




On Mon, Apr 6, 2020 at 9:28 PM Chesnay Schepler <[hidden email]> wrote:

> Nope, it wouldn't work to have it in the root of the project; in a
> practical sense IntelliJ can only really handle them in the standard
> location.
>
> Ironically, the PR currently only works in IntelliJ, because when you
> build the jar with maven we exclude all log4j files via the
> shade-plugin. Which we could of course fix, but we'd need another module
> to override the shade-plugin in to avoid side-effects on
> flink-test-utils-junit.
> I suppose one positive thing would be that we could name the module
> "flink-log4j2-test-configuration" which would at least be a more obvious
> location...
>
> On 06/04/2020 20:53, Chesnay Schepler wrote:
> > Actually, I would first have to double-check whether this would work
> > within IntelliJ...
> >
> > On 06/04/2020 20:40, Chesnay Schepler wrote:
> >> We can also move the file to the root of the project, which should
> >> make it easier to discover.
> >>
> >> flink-test-utils-junit would then just be a distribution vehicle that
> >> few would have to know about.
> >>
> >> On 06/04/2020 13:31, Till Rohrmann wrote:
> >>> Hi Chesnay,
> >>>
> >>> thanks for kicking this discussion off. I agree that deduplicating
> >>> code is
> >>> in general a good idea.
> >>>
> >>> The main benefit seems to be that all modules inherit a
> >>> log4j2-test.properties file and that this file allows to control the
> >>> logging output for several modules.
> >>>
> >>> The main drawback I see is that it complicates the debugging process
> >>> for
> >>> our devs. If you want to debug a problem and need logging for this,
> >>> then
> >>> you have to know that there is a log4j2-test.properties file in
> >>> flink-test-utils-junit which you can tweak. At the moment, it is quite
> >>> straight forward as you simply go to the module which contains the
> >>> test and
> >>> check the resources folder.
> >>>
> >>> If we are ok with this drawback and document the change properly,
> >>> then I'm
> >>> fine with this change.
> >>>
> >>> Cheers,
> >>> Till
> >>>
> >>> On Mon, Apr 6, 2020 at 12:24 PM Chesnay Schepler
> >>> <[hidden email]> wrote:
> >>>
> >>>> Hello,
> >>>>
> >>>> I discovered a handy trick that would allow us to share a single
> >>>> log4j2-test.properties across all modules.
> >>>>
> >>>> https://github.com/apache/flink/pull/11634
> >>>>
> >>>> The file would exist in flink-test-utils-junit/src/main/resources, and
> >>>> be used for all modules except the kafka connectors and yarn-tests
> >>>> (because they have some custom requirements).
> >>>>
> >>>> This would mean the files can no longer go out of sync, utilities
> >>>> can be
> >>>> shared more easily, and you wouldn't need to add a new properties file
> >>>> to new modules (or older ones lacking one) during debugging.
> >>>>
> >>>> Overall I personally quite, but I have heard some concerns about
> >>>> changing dev routines so I wanted to double-check what people think in
> >>>> general.
> >>>>
> >>>>
> >>
> >>
> >
> >
>
>

[Chesnay Schepler-3]

INFO output usually not enough for debugging.

In some cases it may be sufficient, but if you go beyond that then the
dev still has to know to look into one specific module,
bringing us back to square one.

I do think though that a dedicated module for sharing these resources
would actually be fairly straight-forward.

You will always(*except for kafka and yarn-tests) go into this module,
and set it up once at the start of a debugging
session and it will work regardless of which module you move into.

I see this as a clear advantage over the current state.

On 07/04/2020 20:23, Robert Metzger wrote:

> I also like the idea of getting rid of all those copies of the same file
> across our codebase.
>
> How about setting the log level in the log4j config file in
> flink-test-utils-junit/src/main/resources to INFO, and using a separate
> log4j config for local maven runs? (we pass a different log4j file in the
> surefire config (not sure if that really works)).
>
> This way, we don't pollute the command line when building Flink locally
> through maven and developers get INFO log output when running tests from
> the IDE.
> This would reduce the burden of locating and changing the file every time
> one looks into a test.
>
>
>
>
> On Mon, Apr 6, 2020 at 9:28 PM Chesnay Schepler <[hidden email]> wrote:
>
>> Nope, it wouldn't work to have it in the root of the project; in a
>> practical sense IntelliJ can only really handle them in the standard
>> location.
>>
>> Ironically, the PR currently only works in IntelliJ, because when you
>> build the jar with maven we exclude all log4j files via the
>> shade-plugin. Which we could of course fix, but we'd need another module
>> to override the shade-plugin in to avoid side-effects on
>> flink-test-utils-junit.
>> I suppose one positive thing would be that we could name the module
>> "flink-log4j2-test-configuration" which would at least be a more obvious
>> location...
>>
>> On 06/04/2020 20:53, Chesnay Schepler wrote:
>>> Actually, I would first have to double-check whether this would work
>>> within IntelliJ...
>>>
>>> On 06/04/2020 20:40, Chesnay Schepler wrote:
>>>> We can also move the file to the root of the project, which should
>>>> make it easier to discover.
>>>>
>>>> flink-test-utils-junit would then just be a distribution vehicle that
>>>> few would have to know about.
>>>>
>>>> On 06/04/2020 13:31, Till Rohrmann wrote:
>>>>> Hi Chesnay,
>>>>>
>>>>> thanks for kicking this discussion off. I agree that deduplicating
>>>>> code is
>>>>> in general a good idea.
>>>>>
>>>>> The main benefit seems to be that all modules inherit a
>>>>> log4j2-test.properties file and that this file allows to control the
>>>>> logging output for several modules.
>>>>>
>>>>> The main drawback I see is that it complicates the debugging process
>>>>> for
>>>>> our devs. If you want to debug a problem and need logging for this,
>>>>> then
>>>>> you have to know that there is a log4j2-test.properties file in
>>>>> flink-test-utils-junit which you can tweak. At the moment, it is quite
>>>>> straight forward as you simply go to the module which contains the
>>>>> test and
>>>>> check the resources folder.
>>>>>
>>>>> If we are ok with this drawback and document the change properly,
>>>>> then I'm
>>>>> fine with this change.
>>>>>
>>>>> Cheers,
>>>>> Till
>>>>>
>>>>> On Mon, Apr 6, 2020 at 12:24 PM Chesnay Schepler
>>>>> <[hidden email]> wrote:
>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> I discovered a handy trick that would allow us to share a single
>>>>>> log4j2-test.properties across all modules.
>>>>>>
>>>>>> https://github.com/apache/flink/pull/11634
>>>>>>
>>>>>> The file would exist in flink-test-utils-junit/src/main/resources, and
>>>>>> be used for all modules except the kafka connectors and yarn-tests
>>>>>> (because they have some custom requirements).
>>>>>>
>>>>>> This would mean the files can no longer go out of sync, utilities
>>>>>> can be
>>>>>> shared more easily, and you wouldn't need to add a new properties file
>>>>>> to new modules (or older ones lacking one) during debugging.
>>>>>>
>>>>>> Overall I personally quite, but I have heard some concerns about
>>>>>> changing dev routines so I wanted to double-check what people think in
>>>>>> general.
>>>>>>
>>>>>>
>>>>
>>>
>>