Is there a good reason that the M3Reporter creates non daemon threads ?

[agrawaldevesh]

I notice non daemon threads called m3-reporter-%d in my application that come from the use of the M3Reporter. Is there a good reason why its thread factory creates non daemon threads ? This prevents clean JVM exits. Its ofcourse trivial to fix but I would like to understand if there was some intended rationale behind not making them be daemon threads.

This is how the current thread factory used by M3Reporter looks like:

    private static ThreadFactory createThreadFactory() {
        return new ThreadFactory() {
            @Override
            public Thread newThread(Runnable r) {
                return new Thread(r, String.format("m3-reporter-%d", processorThreadCounter.getAndIncrement()));
            }
        };
    }

As you can see it creates non daemon threads. Why ?

[SokolAndrey]

I'm not sure if that was intentional. I don't have context why it was introduced, but the commit message suggests it was done to make "Reporter resilient to crashes of Processor", which could explain usage of non-daemon threads. @alexeykudinkin do you have more context?

[alexeykudinkin]

@agrawaldevesh @SokolAndrey the reason why is that these threads should not be treated as daemon threads: M3Reporter threads have appropriate exit criteria (which requires shutdown method of M3Reporter to be invoked), therefore for clean JVM exit it is required to call shutdown on the reporter, which in turn will make sure that threads will conclude appropriately.

[agrawaldevesh]

HI @alexeykudinkin , can you achieve those semantics in a different way ? For example, do we need to have a "close" or a "flush" call ? What about the use cases where the client is okay with loosing some unflushed metrics but wants to reliably and quickly exit ?

Here is where I am coming from: We use this M3 reporter in Spark apps. We need to wait 1 minute for these threads to go away. This is costly and error prone. We would much rather work with a (possibly configurable) semantic that these are daemon threads that don't block the JVM exit and that we can eagerly flush them on demand if so chosen.

[agrawaldevesh]

Also, I would like to note that just by making them as "non daemon" threads does not guarantee that the shutdown method will be called. So I think a fair plan of action is:

• Make these threads as daemon threads
• Require shutdown to be called prior to thread death or else log a warning.

[alexeykudinkin]

Sure thing, you can make cooldown period configurable and reduce it for your use-case if you need a faster turnaround.

Also, I would like to note that just by making them as "non daemon" threads does not guarantee that the shutdown method will be called. So I think a fair plan of action is:

I did not state this. I stated that they are made non-daemon to make sure that threads don't just randomly quit, when JVM is exiting, and require proper shutdown sequence to be executed.

Make these threads as daemon threads

Let's make sure we're not changing the semantic of the library used by every single Java service at Uber, over-tailoring it for a single use-case. I don't think you've presented convincing arguments why threads should be converted to be daemons in general case, so i'd much rather keep existing semantic guaranteeing proper metrics delivery during shutdown.

[agrawaldevesh]

Fair enough. I will make the daemon threads be opt in and user configurable, such that they do not get activated for each service. Good point on reducing the blast radius if this change.

On Fri, Jan 7, 2022 at 11:59 AM Alexey Kudinkin @.***> wrote:

Sure thing, you can make cooldown period configurable and reduce it for your use-case if you need a faster turnaround.

Also, I would like to note that just by making them as "non daemon" threads does not guarantee that the shutdown method will be called. So I think a fair plan of action is:

I did not state this. I stated that they are made non-daemon to make sure that threads don't just randomly quit, when JVM is exiting, and require proper shutdown sequence to be executed.

Make these threads as daemon threads

Let's make sure we're not changing the semantic of the library used by every single Java service at Uber, over-tailoring it for a single use-case. I don't think you've presented convincing arguments why threads should be converted to be daemons in general case, so i'd much rather keep existing semantic guaranteeing proper metrics delivery during shutdown.

— Reply to this email directly, view it on GitHub https://github.com/uber-java/tally/issues/102#issuecomment-1007697240, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE44R725FRA3NGOYKDCATNTUU5AZVANCNFSM5KUGOUUA . You are receiving this because you were mentioned.Message ID: @.***>