ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexei Scherbakov <alexey.scherbak...@gmail.com>
Subject Re: [DISSCUSSION] Common logger interface.
Date Thu, 08 Apr 2021 11:15:52 GMT
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 <andrey.mashenkov@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

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