----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55591/#review161791 -----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/logging/log4j/AlertAppender.java (line 68) <https://reviews.apache.org/r/55591/#comment233098> addConnectListener also takes "existingSystemsLock", so this may run into same issue? - Hitesh Khamesra On Jan. 16, 2017, 10 p.m., Kirk Lund wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55591/ > ----------------------------------------------------------- > > (Updated Jan. 16, 2017, 10 p.m.) > > > Review request for geode, Bruce Schuchardt, Darrel Schneider, Jinmei Liao, > Jared Stewart, and Kevin Duling. > > > Bugs: GEODE-2297 > https://issues.apache.org/jira/browse/GEODE-2297 > > > Repository: geode > > > Description > ------- > > GEODE-2297: passively discover DistributedSystem instance in AlertAppender to > use in append > > AlertAppender was actively hitting a synchronization and fetching > InternalDistributedSystem singleton in every append(!) call. This can produce > a deadlock if an alert is generated during startup. > > This change removes the active fetch from the append code path and uses > InternalDistributedSystem.ConnectListener and > InternalDistributedSystem.DisconnectListener to receive notifications of > connect and disconnect. These notifications set an AtomicReference that is > now checked within append. (Registering a ConnectListener is static and > remains registered for the life of the JVM. For every onConnect, the > AlertAppender registers a non-static DisconnectListener for each > InternalDistributedSystem instance.) > > I also fixed some minor IDE warnings, changed a logger statement from info to > debug and added a TODO to the class' logger instance. I want to follow up > later to *probably* change from using Logger to StatusLogger. Note: we run > with StatusLogger level of FATAL by default. Appender impl's really should > use StatusLogger instead of Logger (to avoid risk of infinite looping). As > it's currently written (with Logger), you'd have to set the AlertLevel to > DEBUG to hit this problem. I believe someone would only do this by error or > for experimentation. > > > Diffs > ----- > > > geode-core/src/main/java/org/apache/geode/internal/logging/log4j/AlertAppender.java > a6c54aba352ba4dd3ece94c94f770292db6e9284 > > Diff: https://reviews.apache.org/r/55591/diff/ > > > Testing > ------- > > precheckin in progress > > > Thanks, > > Kirk Lund > >
