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 < [email protected]> 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 <[email protected]>: > > > 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 < > > [email protected]> > > 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 < > > > [email protected]> 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 < > > [email protected] > > >> >: > > >> > > >> > 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 Вячеслав Коптилин < > > >> > [email protected]> > > >> > 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 < > > >> > > [email protected] > > >> > > >: > > >> > > > > >> > > > 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 < > > >> > > [email protected] > > >> > > > >: > > >> > > > > > >> > > > > 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 < > > >> > > > > [email protected]> > > >> > > > > 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
