Re: [DISSCUSSION] Common logger interface.

[Andrey Mashenkov <an...@gmail.com>]

Alex,

Sorry, I've just found your question.

> Is it really need to use thread-locals for user threads? - probably not.

Your understanding is right, we can share a single logger instance for all
non-Ignite threads.

Igniters,
I've created a ticket [1] to separate logs from different nodes.

There are several possible ways.
1. Switch back to per-node logger which looks like an unwanted option.
2. Use a wrapper instead of a real logger in static fields, which will
switch between default logger instance or per-node logger instance.
Per-node instance can be a kind of thread-local object. I suggest using
extend Thread class with IgniteThread class which will have
an additional field for a logger
and force using IgniteThread for all internal threads.
Then we should choose a preferable way per-node logger will work:
2.1 Per-node logger adds a node-specific prefix to log messages to make
possible distinct logs from different nodes. This will work like in
Ignite-2.
2.2 Per-node logger can be configured independently (the way we should
suggest as well), and e.g. may even support a table of loggers for every
known logging category.

Any thoughts?

[1] https://issues.apache.org/jira/browse/IGNITE-14897

On Thu, Apr 8, 2021 at 2:16 PM Alexei Scherbakov <
alexey.scherbakoff@gmail.com> wrote:

> Andrey,
>
> The PR looks good to me.
>
> Maybe we can wrap all internal threads into IgniteThread - I'm almost sure
> this will work in this way.
>
> Is it really need to use thread-locals for user threads? - probably not.
> I'm not sure if there is any problem at all. As soon as we want to have
> async API everywhere, out code should not be executed in user thread
>
> чт, 8 апр. 2021 г. в 13:37, Andrey Mashenkov <an...@gmail.com>:
>
> > Also, with the suggested approach,
> > we should avoid indirectly usage of ForkJoinPool internally or set our
> own
> > pool instance explicitly when using reactive things.
> >
> > On Thu, Apr 8, 2021 at 1:33 PM Andrey Mashenkov <
> > andrey.mashenkov@gmail.com>
> > wrote:
> >
> > > Alexey,
> > >
> > > I've made a PR for logger [1].
> > > Seems, we will need 2 logger classes.
> > > 1. Node-aware logger adapter, that will add node prefix to messages and
> > > delegate calls to System.Logger or whatever.
> > > 2. Logger wrapper that will get logger from a thread-local.
> > >
> > > I don't like to use ThreadLocal directly when possible.
> > > Maybe we can wrap all internal threads into IgniteThread and keep the
> > > logger in an IgniteThread field to avoid lookups into thread-local-map.
> > >
> > > For user threads, only ThreadLocals can be used.
> > > Is it really need to use thread-locals for user threads?
> > > Will it be always obvious which node exception was thrown on? Any kind
> of
> > > embedded mode?
> > >
> > > [1] https://github.com/apache/ignite-3/pull/87
> > >
> > > On Thu, Apr 8, 2021 at 12:32 PM Alexei Scherbakov <
> > > alexey.scherbakoff@gmail.com> wrote:
> > >
> > >> Andrey,
> > >>
> > >> *final* word in the example is missing, my bad.
> > >>
> > >> I like the static logger approach.
> > >>
> > >> Regarding your comments:
> > >> * The static logger can easily be used by multiple nodes in a single
> > JVM,
> > >> it's a matter of implementation. It can be achieved by setting thread
> > >> local
> > >> logger context for the node.
> > >> For user threads the context can be set while entering ignite context
> > (for
> > >> example, by calling public API method)
> > >> * Factory method is not necessary, because we already have a proxy
> > object
> > >> -
> > >> LogWrapper, hiding internal implementation.
> > >>
> > >>
> > >>
> > >>
> > >>
> > >>
> > >>
> > >> ср, 7 апр. 2021 г. в 18:39, Andrey Mashenkov <
> > andrey.mashenkov@gmail.com
> > >> >:
> > >>
> > >> > Alexei,
> > >> >
> > >> > I see you've merged LoggerWrapper into main and use it in Raft
> module.
> > >> > I can't figure out what manner you suggest to use LoggerWrapper in.
> > >> > In the example above 'LOG' field is static, non-final and you
> create a
> > >> > wrapper explicitly.
> > >> >
> > >> > I see 2 ways:
> > >> > * Use a factory method to create a logger and set it into 'static
> > final'
> > >> > field.
> > >> > In this case, a user will not be able to split logs from different
> > nodes
> > >> > running in the same JVM.
> > >> > * Set logger into non-static field (with dependency injection
> future).
> > >> > In this case, we need to pass the logger to every class instance
> where
> > >> it
> > >> > can be used.
> > >> >
> > >> >
> > >> > On Fri, Mar 26, 2021 at 6:48 PM Вячеслав Коптилин <
> > >> > slava.koptilin@gmail.com>
> > >> > wrote:
> > >> >
> > >> > > Hello Alexei,
> > >> > >
> > >> > > It would be nice to add something like as follows:
> > >> > >     boolean isInfoEnabled();
> > >> > >     boolean isDebugEnabled();
> > >> > > or
> > >> > >     boolean isLoggable(Level) - the same way which System.Logger
> > >> suggests
> > >> > >
> > >> > > Thanks,
> > >> > > S.
> > >> > >
> > >> > > пт, 26 мар. 2021 г. в 17:41, Alexei Scherbakov <
> > >> > > alexey.scherbakoff@gmail.com
> > >> > > >:
> > >> > >
> > >> > > > Andrey,
> > >> > > >
> > >> > > > I've introduced a new class LogWrapper to fix usability issues
> [1]
> > >> > > >
> > >> > > > The suggested usage is something like:
> > >> > > >
> > >> > > > private static LogWrapper LOG = new LogWrapper(MyClass.class);
> > >> > > >
> > >> > > > [1]
> > >> > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> >
> https://github.com/gridgain/apache-ignite-3/blob/9acb050a6a6a601ead849797293a1d0ad48ab9e0/modules/core/src/main/java/org/apache/ignite/lang/LogWrapper.java
> > >> > > >
> > >> > > > пт, 26 мар. 2021 г. в 16:05, Andrey Mashenkov <
> > >> > > andrey.mashenkov@gmail.com
> > >> > > > >:
> > >> > > >
> > >> > > > > Forgot to attach a link to the PR with an example [1].
> > >> > > > >
> > >> > > > > [1] https://github.com/apache/ignite-3/pull/59
> > >> > > > >
> > >> > > > > On Fri, Mar 26, 2021 at 4:03 PM Andrey Mashenkov <
> > >> > > > > andrey.mashenkov@gmail.com>
> > >> > > > > wrote:
> > >> > > > >
> > >> > > > > > Hi Igniters,
> > >> > > > > >
> > >> > > > > > In almost every new task we faced the problem of what logger
> > >> has to
> > >> > > be
> > >> > > > > > used: JUL. log4J or any else.
> > >> > > > > >
> > >> > > > > > Since JDK 9 there is a System.Logger which interface looks
> > >> > acceptable
> > >> > > > for
> > >> > > > > > use,
> > >> > > > > > excepts maybe some usability issues like method signatures.
> > >> > > > > > LogLevel is passed as a mandatory argument, and no shortcut
> > >> methods
> > >> > > are
> > >> > > > > > provided (like 'warn', 'error' or 'info').
> > >> > > > > >
> > >> > > > > > I like Alex Scherbakov idea [1] to use a brand new JDK
> system
> > >> > logger
> > >> > > by
> > >> > > > > > default and
> > >> > > > > > extend it with shortcut methods.
> > >> > > > > >
> > >> > > > > > I've created a ticket to unify logger usage in Ignite-3.0
> > >> project
> > >> > to
> > >> > > > fix
> > >> > > > > > already existed code.
> > >> > > > > >
> > >> > > > > > Any thoughts or objections?
> > >> > > > > >
> > >> > > > > > --
> > >> > > > > > Best regards,
> > >> > > > > > Andrey V. Mashenkov
> > >> > > > > >
> > >> > > > >
> > >> > > > >
> > >> > > > > --
> > >> > > > > Best regards,
> > >> > > > > Andrey V. Mashenkov
> > >> > > > >
> > >> > > >
> > >> > > >
> > >> > > > --
> > >> > > >
> > >> > > > Best regards,
> > >> > > > Alexei Scherbakov
> > >> > > >
> > >> > >
> > >> >
> > >> >
> > >> > --
> > >> > Best regards,
> > >> > Andrey V. Mashenkov
> > >> >
> > >>
> > >>
> > >> --
> > >>
> > >> Best regards,
> > >> Alexei Scherbakov
> > >>
> > >
> > >
> > > --
> > > Best regards,
> > > Andrey V. Mashenkov
> > >
> >
> >
> > --
> > Best regards,
> > Andrey V. Mashenkov
> >
>
>
> --
>
> Best regards,
> Alexei Scherbakov
>


-- 
Best regards,
Andrey V. Mashenkov