You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ignite.apache.org by Andrey Mashenkov <an...@gmail.com> on 2021/04/07 15:39:12 UTC
Re: [DISSCUSSION] Common logger interface.
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 Вячеслав Коптилин <sl...@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
Re: [DISSCUSSION] Common logger interface.
Posted by 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
Re: [DISSCUSSION] Common logger interface.
Posted by Alexei Scherbakov <al...@gmail.com>.
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
Re: [DISSCUSSION] Common logger interface.
Posted by 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 <an...@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
Re: [DISSCUSSION] Common logger interface.
Posted by Andrey Mashenkov <an...@gmail.com>.
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 <an...@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
Re: [DISSCUSSION] Common logger interface.
Posted by Alexei Scherbakov <al...@gmail.com>.
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 <an...@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