ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrey Mashenkov <andrey.mashen...@gmail.com>
Subject Re: [DISSCUSSION] Common logger interface.
Date Thu, 08 Apr 2021 10:33:42 GMT
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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message