[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

classic Classic list List threaded Threaded
35 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
GitHub user ctubbsii opened a pull request:

    https://github.com/apache/accumulo/pull/223

    ACCUMULO-4409 Create AccumuloMonitorAppender

    Creates an AccumuloMonitorAppender, and does additional logging system cleanup in the files and scripts.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/ctubbsii/accumulo ACCUMULO-4409-custom-appender

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/accumulo/pull/223.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #223
   
----
commit cac1a3f9cf6fa9a39ce5657328941c1794ce10a7
Author: Christopher Tubbs <[hidden email]>
Date:   2017-02-24T03:55:24Z

    ACCUMULO-4409 Create AccumuloMonitorAppender

commit 6af8bcb96f9b506043fc745406f8a13da011a2f1
Author: Christopher Tubbs <[hidden email]>
Date:   2017-02-28T23:43:21Z

    ACCUMULO-4409 More logging bootstrap cleanup
   
    Clean up unneeded system properties, environment variables, logging
    configuration, and related classes.
   
    Fix AccumuloMonitorAppender to reload when monitor restarts in the same
    location.

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
Github user mikewalch commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103702178
 
    --- Diff: assemble/conf/log4j-monitor.properties ---
    @@ -16,23 +16,21 @@
     ## Log4j 1.2 file that configures logging for Accumulo Monitor
     ## The system properties referenced below are configured by accumulo-env.sh
     
    -## Write out INFO and higher to log file
    +## Define a log file appender
     log4j.appender.file=org.apache.log4j.RollingFileAppender
    -log4j.appender.file.File=${accumulo.log.dir}/${accumulo.service.id}.log
    +log4j.appender.file.File=${accumulo.log.dir}/${accumulo.application}.log
     log4j.appender.file.MaxFileSize=100MB
     log4j.appender.file.MaxBackupIndex=10
     log4j.appender.file.Threshold=INFO
     log4j.appender.file.layout=org.apache.log4j.PatternLayout
     log4j.appender.file.layout.ConversionPattern=%d{ISO8601} [%-8c{2}] %-5p: %m%n
     
    -## Keep the last few log messages for display to the user in Monitor GUI
    +## Define an appender for the Accumulo Monitor to log to its own web GUI
     log4j.appender.gui=org.apache.accumulo.server.monitor.LogService
    -log4j.appender.gui.Keep=50
     log4j.appender.gui.Threshold=WARN
     
    -## Log accumulo messages to file and gui appenders
    -log4j.logger.org.apache.accumulo=INFO, file, gui
    -log4j.additivity.org.apache.accumulo=false
    +## Append monitor logs to its own web GUI
    +log4j.logger.org.apache.accumulo=INHERITED, gui
    --- End diff --
   
    This is cool.  I am guessing `INHERITED` takes level of higher logger in tree. Is there documentation on this?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user mikewalch commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103707238
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/util/AccumuloMonitorAppender.java ---
    @@ -0,0 +1,153 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.monitor.util;
    +
    +import static java.nio.charset.StandardCharsets.UTF_8;
    +
    +import java.util.concurrent.Executors;
    +import java.util.concurrent.ScheduledExecutorService;
    +import java.util.concurrent.TimeUnit;
    +import java.util.concurrent.atomic.AtomicBoolean;
    +
    +import org.apache.accumulo.core.Constants;
    +import org.apache.accumulo.core.client.Instance;
    +import org.apache.accumulo.core.conf.Property;
    +import org.apache.accumulo.core.zookeeper.ZooUtil;
    +import org.apache.accumulo.fate.zookeeper.ZooCache;
    +import org.apache.accumulo.fate.zookeeper.ZooCacheFactory;
    +import org.apache.accumulo.server.client.HdfsZooInstance;
    +import org.apache.log4j.AsyncAppender;
    +import org.apache.log4j.net.SocketAppender;
    +import org.apache.zookeeper.data.Stat;
    +
    +import com.google.common.net.HostAndPort;
    +
    +public class AccumuloMonitorAppender extends AsyncAppender {
    --- End diff --
   
    Did you do any testing to verify that this class will switch to new monitor?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user mikewalch commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103706254
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/watcher/Log4jConfiguration.java ---
    @@ -1,59 +0,0 @@
    -/*
    - * Licensed to the Apache Software Foundation (ASF) under one or more
    - * contributor license agreements.  See the NOTICE file distributed with
    - * this work for additional information regarding copyright ownership.
    - * The ASF licenses this file to You under the Apache License, Version 2.0
    - * (the "License"); you may not use this file except in compliance with
    - * the License.  You may obtain a copy of the License at
    - *
    - *     http://www.apache.org/licenses/LICENSE-2.0
    - *
    - * Unless required by applicable law or agreed to in writing, software
    - * distributed under the License is distributed on an "AS IS" BASIS,
    - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    - * See the License for the specific language governing permissions and
    - * limitations under the License.
    - */
    -package org.apache.accumulo.server.watcher;
    -
    -import static java.util.Objects.requireNonNull;
    -
    -import java.io.File;
    -
    -import org.apache.log4j.LogManager;
    -import org.apache.log4j.PropertyConfigurator;
    -import org.apache.log4j.xml.DOMConfigurator;
    -
    -/**
    - * Encapsulate calls to PropertyConfigurator or DOMConfigurator to set up logging
    - */
    -public class Log4jConfiguration {
    --- End diff --
   
    Nice to see this removed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user mikewalch commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103705868
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/Accumulo.java ---
    @@ -107,18 +104,6 @@ public static synchronized Path getAccumuloInstanceIdPath(VolumeManager fs) {
     
       public static void init(VolumeManager fs, ServerConfigurationFactory serverConfig, String application) throws IOException {
         final AccumuloConfiguration conf = serverConfig.getConfiguration();
    -    final Instance instance = serverConfig.getInstance();
    -
    -    String logConfigFile = System.getProperty("log4j.configuration");
    -    if (logConfigFile != null) {
    -      if (logConfigFile.startsWith("file:")) {
    --- End diff --
   
    Good thing I didn't fix this :-)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user mikewalch commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103702937
 
    --- Diff: assemble/conf/log4j-service.properties ---
    @@ -13,43 +13,47 @@
     # See the License for the specific language governing permissions and
     # limitations under the License.
     
    -## Log4j 1.2 file that configures logging for all Accumulo services (Master, TabletServer, GC & Tracer) except Monitor
    +## Log4j 1.2 file that configures logging for all Accumulo services (Master, TabletServer, GC, and Tracer) except Monitor
     ## The system properties referenced below are configured by accumulo-env.sh
     
    -## Write out INFO and higher to log file
    +## Define a log file appender
     log4j.appender.file=org.apache.log4j.RollingFileAppender
    -log4j.appender.file.File=${accumulo.log.dir}/${accumulo.service.id}.log
    +log4j.appender.file.File=${accumulo.log.dir}/${accumulo.application}.log
     log4j.appender.file.MaxFileSize=100MB
     log4j.appender.file.MaxBackupIndex=10
     log4j.appender.file.Threshold=INFO
     log4j.appender.file.layout=org.apache.log4j.PatternLayout
     log4j.appender.file.layout.ConversionPattern=%d{ISO8601} [%-8c{2}] %-5p: %m%n
     
    -## Forwards Accumulo messages at WARN or higher to Accumulo Monitor
    -log4j.appender.monitor=org.apache.accumulo.start.util.AsyncSocketAppender
    -log4j.appender.monitor.RemoteHost=${accumulo.monitor.host}
    -log4j.appender.monitor.Port=${accumulo.monitor.log.port}
    -log4j.appender.monitor.Application=${accumulo.service.id}
    +## Define an appender to send important logs to the the primary Accumulo Monitor
    +## The primary monitor is the one currently holding a shared lock in ZooKeeper,
    +## and is typically the one that started first.
    +log4j.appender.monitor=org.apache.accumulo.monitor.util.AccumuloMonitorAppender
     log4j.appender.monitor.Threshold=WARN
     
    -## Configures Audit logs which are OFF by default.
    +## Uncomment to define a log file appender for audit logs
     #log4j.appender.audit=org.apache.log4j.DailyRollingFileAppender
    -#log4j.appender.audit.File=${accumulo.log.dir}/${accumulo.audit.log}
    +#log4j.appender.audit.File=${accumulo.log.dir}/${accumulo.application}.audit
    --- End diff --
   
    I am OK with this but it changes behaviour of audit log.  Previously, there was one audit log per node that was shared by all services.  This will create an audit log for each service.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user mikewalch commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103705555
 
    --- Diff: assemble/conf/templates/accumulo-env.sh ---
    @@ -53,20 +58,37 @@ shell)   JAVA_OPTS=("${JAVA_OPTS[@]}" ${shellHigh_shellLow}) ;;
     *)       JAVA_OPTS=("${JAVA_OPTS[@]}" ${otherHigh_otherLow}) ;;
     esac
     
    -## JVM options set for logging.  Review logj4 properties files to see how they are used.
    -JAVA_OPTS=("${JAVA_OPTS[@]}"
    +## JVM options set for logging. Review logj4 properties files to see how they are used.
    +JAVA_OPTS=("${JAVA_OPTS[@]}"
       "-Daccumulo.log.dir=${ACCUMULO_LOG_DIR}"
    -  "-Daccumulo.service.id=${ACCUMULO_CMD}${ACCUMULO_SERVICE_INSTANCE}_$(hostname)"
    -  "-Daccumulo.audit.log=$(hostname).audit")
    +  "-Daccumulo.application=${ACCUMULO_CMD}${ACCUMULO_SERVICE_INSTANCE}_$(hostname)")
     
     case "$ACCUMULO_CMD" in
    -monitor)                    JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=file:${ACCUMULO_CONF_DIR}/log4j-monitor.properties") ;;
    -gc|master|tserver|tracer)   JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=file:${ACCUMULO_CONF_DIR}/log4j-service.properties") ;;
    -*)                          JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=file:${ACCUMULO_CONF_DIR}/log4j.properties") ;;
    +  monitor)
    +    JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=log4j-monitor.properties")
    +    ;;
    +  gc|master|tserver|tracer)
    +    JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=log4j-service.properties")
    +    ;;
    +  *)
    +    # let log4j use its default behavior (log4j.xml, log4j.properties)
    +    true
    +    ;;
     esac
     
     export JAVA_OPTS
     
    +## External class path items for Java system class loader (dependencies not included with Accumulo)
    --- End diff --
   
    With this added, I think default settings for `general.classpaths` should be removed from accumulo-site.xml and `accumulo-util create-config` should no longer configure classpaths.  I think we could have another section or appendix in the user manual for configuring Accumulo to use vendor-specific releases of Hadoop & Zookeeper.  There could be a comment here pointing users to that section if they are using a vendor release of Hadoop or Zookeeper.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103719146
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/util/AccumuloMonitorAppender.java ---
    @@ -0,0 +1,153 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.monitor.util;
    +
    +import static java.nio.charset.StandardCharsets.UTF_8;
    +
    +import java.util.concurrent.Executors;
    +import java.util.concurrent.ScheduledExecutorService;
    +import java.util.concurrent.TimeUnit;
    +import java.util.concurrent.atomic.AtomicBoolean;
    +
    +import org.apache.accumulo.core.Constants;
    +import org.apache.accumulo.core.client.Instance;
    +import org.apache.accumulo.core.conf.Property;
    +import org.apache.accumulo.core.zookeeper.ZooUtil;
    +import org.apache.accumulo.fate.zookeeper.ZooCache;
    +import org.apache.accumulo.fate.zookeeper.ZooCacheFactory;
    +import org.apache.accumulo.server.client.HdfsZooInstance;
    +import org.apache.log4j.AsyncAppender;
    +import org.apache.log4j.net.SocketAppender;
    +import org.apache.zookeeper.data.Stat;
    +
    +import com.google.common.net.HostAndPort;
    +
    +public class AccumuloMonitorAppender extends AsyncAppender {
    +
    +  private final ScheduledExecutorService executorService;
    +  private final AtomicBoolean trackerScheduled;
    +
    +  /**
    +   * A Log4j Appender which follows the registered location of the active Accumulo monitor service, and forwards log messages to it
    +   */
    +  public AccumuloMonitorAppender() {
    +    // create the background thread to watch for updates to monitor location
    +    trackerScheduled = new AtomicBoolean(false);
    +    executorService = Executors.newSingleThreadScheduledExecutor(runnable -> {
    +      Thread t = new Thread(runnable, AccumuloMonitorAppender.class.getSimpleName() + " Location Tracker");
    +      t.setDaemon(true);
    +      return t;
    --- End diff --
   
    Don't we have some `Daemon` class which encapsulates some of this logic already? (thread naming and doing `setDaemon(true)`)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103718100
 
    --- Diff: assemble/conf/log4j-monitor.properties ---
    @@ -16,23 +16,21 @@
     ## Log4j 1.2 file that configures logging for Accumulo Monitor
     ## The system properties referenced below are configured by accumulo-env.sh
     
    -## Write out INFO and higher to log file
    +## Define a log file appender
     log4j.appender.file=org.apache.log4j.RollingFileAppender
    -log4j.appender.file.File=${accumulo.log.dir}/${accumulo.service.id}.log
    +log4j.appender.file.File=${accumulo.log.dir}/${accumulo.application}.log
     log4j.appender.file.MaxFileSize=100MB
     log4j.appender.file.MaxBackupIndex=10
     log4j.appender.file.Threshold=INFO
     log4j.appender.file.layout=org.apache.log4j.PatternLayout
     log4j.appender.file.layout.ConversionPattern=%d{ISO8601} [%-8c{2}] %-5p: %m%n
     
    -## Keep the last few log messages for display to the user in Monitor GUI
    +## Define an appender for the Accumulo Monitor to log to its own web GUI
    --- End diff --
   
    Hooray comments!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103718041
 
    --- Diff: assemble/bin/accumulo ---
    @@ -127,13 +127,12 @@ function main() {
         JAVA=($ACCUMULO_JAVA_PREFIX $JAVA)
       fi
       
    -  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
     
    -  exec "${JAVA[@]}" "-Dapp=$ACCUMULO_CMD" \
    +  CLASSPATH="${conf}:${lib}/*:${CLASSPATH}"
    --- End diff --
   
    Nit: do a `-z "${CLASSPATH}"` first? Saves that unnecessary colon in what would be the common case


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103720637
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/util/AccumuloMonitorAppender.java ---
    @@ -0,0 +1,153 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.monitor.util;
    +
    +import static java.nio.charset.StandardCharsets.UTF_8;
    +
    +import java.util.concurrent.Executors;
    +import java.util.concurrent.ScheduledExecutorService;
    +import java.util.concurrent.TimeUnit;
    +import java.util.concurrent.atomic.AtomicBoolean;
    +
    +import org.apache.accumulo.core.Constants;
    +import org.apache.accumulo.core.client.Instance;
    +import org.apache.accumulo.core.conf.Property;
    +import org.apache.accumulo.core.zookeeper.ZooUtil;
    +import org.apache.accumulo.fate.zookeeper.ZooCache;
    +import org.apache.accumulo.fate.zookeeper.ZooCacheFactory;
    +import org.apache.accumulo.server.client.HdfsZooInstance;
    +import org.apache.log4j.AsyncAppender;
    +import org.apache.log4j.net.SocketAppender;
    +import org.apache.zookeeper.data.Stat;
    +
    +import com.google.common.net.HostAndPort;
    +
    +public class AccumuloMonitorAppender extends AsyncAppender {
    +
    +  private final ScheduledExecutorService executorService;
    +  private final AtomicBoolean trackerScheduled;
    +
    +  /**
    +   * A Log4j Appender which follows the registered location of the active Accumulo monitor service, and forwards log messages to it
    +   */
    +  public AccumuloMonitorAppender() {
    +    // create the background thread to watch for updates to monitor location
    +    trackerScheduled = new AtomicBoolean(false);
    +    executorService = Executors.newSingleThreadScheduledExecutor(runnable -> {
    +      Thread t = new Thread(runnable, AccumuloMonitorAppender.class.getSimpleName() + " Location Tracker");
    +      t.setDaemon(true);
    +      return t;
    +    });
    +  }
    +
    +  @Override
    +  public void activateOptions() {
    +    // only schedule it once (in case options get activated more than once); not sure if this is possible
    +    if (trackerScheduled.compareAndSet(false, true)) {
    +      // wait 5 seconds, then run every 5 seconds
    +      executorService.scheduleAtFixedRate(new MonitorTracker(), 5, 5, TimeUnit.SECONDS);
    +    }
    +    super.activateOptions();
    +  }
    +
    +  @Override
    +  public void close() {
    +    if (!executorService.isShutdown()) {
    +      executorService.shutdownNow();
    +    }
    +    super.close();
    +  }
    +
    +  private class MonitorTracker implements Runnable {
    +
    +    private String path;
    +    private ZooCache zooCache;
    +
    +    private long lastModifiedTransactionId;
    +    private SocketAppender lastSocketAppender;
    +
    +    public MonitorTracker() {
    +
    +      // path and zooCache are lazily set the first time this tracker is run
    +      // this allows the tracker to be constructed and scheduled during log4j initialization without
    +      // triggering any actual logs from the Accumulo or ZooKeeper code
    +      this.path = null;
    +      this.zooCache = null;
    +
    +      this.lastModifiedTransactionId = 0;
    +      this.lastSocketAppender = null;
    +    }
    +
    +    @Override
    +    public void run() {
    --- End diff --
   
    No tests for this new code? :wink:


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103718621
 
    --- Diff: assemble/conf/templates/accumulo-env.sh ---
    @@ -53,20 +58,37 @@ shell)   JAVA_OPTS=("${JAVA_OPTS[@]}" ${shellHigh_shellLow}) ;;
     *)       JAVA_OPTS=("${JAVA_OPTS[@]}" ${otherHigh_otherLow}) ;;
     esac
     
    -## JVM options set for logging.  Review logj4 properties files to see how they are used.
    -JAVA_OPTS=("${JAVA_OPTS[@]}"
    +## JVM options set for logging. Review logj4 properties files to see how they are used.
    +JAVA_OPTS=("${JAVA_OPTS[@]}"
       "-Daccumulo.log.dir=${ACCUMULO_LOG_DIR}"
    -  "-Daccumulo.service.id=${ACCUMULO_CMD}${ACCUMULO_SERVICE_INSTANCE}_$(hostname)"
    -  "-Daccumulo.audit.log=$(hostname).audit")
    +  "-Daccumulo.application=${ACCUMULO_CMD}${ACCUMULO_SERVICE_INSTANCE}_$(hostname)")
     
     case "$ACCUMULO_CMD" in
    -monitor)                    JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=file:${ACCUMULO_CONF_DIR}/log4j-monitor.properties") ;;
    -gc|master|tserver|tracer)   JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=file:${ACCUMULO_CONF_DIR}/log4j-service.properties") ;;
    -*)                          JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=file:${ACCUMULO_CONF_DIR}/log4j.properties") ;;
    +  monitor)
    +    JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=log4j-monitor.properties")
    +    ;;
    +  gc|master|tserver|tracer)
    +    JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=log4j-service.properties")
    +    ;;
    +  *)
    +    # let log4j use its default behavior (log4j.xml, log4j.properties)
    +    true
    +    ;;
     esac
     
     export JAVA_OPTS
     
    +## External class path items for Java system class loader (dependencies not included with Accumulo)
    +CLASSPATH="$(find "$ZOOKEEPER_HOME"/{,lib} "$HADOOP_PREFIX"/share/hadoop/{common,common/lib,hdfs,mapreduce,yarn} -maxdepth 1 -name '*.jar' \
    +  -and -not -name '*slf4j*' \
    +  -and -not -name '*fatjar*' \
    +  -and -not -name '*-javadoc*' \
    +  -and -not -name '*-sources*.jar' \
    +  -and -not -name '*-test*.jar' \
    +  -print0 | tr '\0' ':')$CLASSPATH"
    --- End diff --
   
    This seems like a strange change to be included in this pull request..


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103732845
 
    --- Diff: assemble/conf/log4j-monitor.properties ---
    @@ -16,23 +16,21 @@
     ## Log4j 1.2 file that configures logging for Accumulo Monitor
     ## The system properties referenced below are configured by accumulo-env.sh
     
    -## Write out INFO and higher to log file
    +## Define a log file appender
     log4j.appender.file=org.apache.log4j.RollingFileAppender
    -log4j.appender.file.File=${accumulo.log.dir}/${accumulo.service.id}.log
    +log4j.appender.file.File=${accumulo.log.dir}/${accumulo.application}.log
     log4j.appender.file.MaxFileSize=100MB
     log4j.appender.file.MaxBackupIndex=10
     log4j.appender.file.Threshold=INFO
     log4j.appender.file.layout=org.apache.log4j.PatternLayout
     log4j.appender.file.layout.ConversionPattern=%d{ISO8601} [%-8c{2}] %-5p: %m%n
     
    -## Keep the last few log messages for display to the user in Monitor GUI
    +## Define an appender for the Accumulo Monitor to log to its own web GUI
     log4j.appender.gui=org.apache.accumulo.server.monitor.LogService
    -log4j.appender.gui.Keep=50
     log4j.appender.gui.Threshold=WARN
     
    -## Log accumulo messages to file and gui appenders
    -log4j.logger.org.apache.accumulo=INFO, file, gui
    -log4j.additivity.org.apache.accumulo=false
    +## Append monitor logs to its own web GUI
    +log4j.logger.org.apache.accumulo=INHERITED, gui
    --- End diff --
   
    Yes, log4j documents "INHERITED" and "NULL" as synonyms for the default behavior of inheriting from the parent logger. It'd also be fine if we left it as "INFO", but I figured it'd make more sense to default to the user needing to adjust it in fewer places if they wished to change it.
   
    INHERITED here means that it will inherit "INFO" from the root logger, but the `gui` appender is still going to filter out anything below "WARN".


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user mikewalch commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103733316
 
    --- Diff: assemble/conf/templates/accumulo-env.sh ---
    @@ -53,20 +58,37 @@ shell)   JAVA_OPTS=("${JAVA_OPTS[@]}" ${shellHigh_shellLow}) ;;
     *)       JAVA_OPTS=("${JAVA_OPTS[@]}" ${otherHigh_otherLow}) ;;
     esac
     
    -## JVM options set for logging.  Review logj4 properties files to see how they are used.
    -JAVA_OPTS=("${JAVA_OPTS[@]}"
    +## JVM options set for logging. Review logj4 properties files to see how they are used.
    +JAVA_OPTS=("${JAVA_OPTS[@]}"
       "-Daccumulo.log.dir=${ACCUMULO_LOG_DIR}"
    -  "-Daccumulo.service.id=${ACCUMULO_CMD}${ACCUMULO_SERVICE_INSTANCE}_$(hostname)"
    -  "-Daccumulo.audit.log=$(hostname).audit")
    +  "-Daccumulo.application=${ACCUMULO_CMD}${ACCUMULO_SERVICE_INSTANCE}_$(hostname)")
     
     case "$ACCUMULO_CMD" in
    -monitor)                    JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=file:${ACCUMULO_CONF_DIR}/log4j-monitor.properties") ;;
    -gc|master|tserver|tracer)   JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=file:${ACCUMULO_CONF_DIR}/log4j-service.properties") ;;
    -*)                          JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=file:${ACCUMULO_CONF_DIR}/log4j.properties") ;;
    +  monitor)
    +    JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=log4j-monitor.properties")
    +    ;;
    +  gc|master|tserver|tracer)
    +    JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=log4j-service.properties")
    +    ;;
    +  *)
    +    # let log4j use its default behavior (log4j.xml, log4j.properties)
    +    true
    +    ;;
     esac
     
     export JAVA_OPTS
     
    +## External class path items for Java system class loader (dependencies not included with Accumulo)
    +CLASSPATH="$(find "$ZOOKEEPER_HOME"/{,lib} "$HADOOP_PREFIX"/share/hadoop/{common,common/lib,hdfs,mapreduce,yarn} -maxdepth 1 -name '*.jar' \
    +  -and -not -name '*slf4j*' \
    +  -and -not -name '*fatjar*' \
    +  -and -not -name '*-javadoc*' \
    +  -and -not -name '*-sources*.jar' \
    +  -and -not -name '*-test*.jar' \
    +  -print0 | tr '\0' ':')$CLASSPATH"
    --- End diff --
   
    He's doing this because log4j tries to initialize his new appender and will throw a ClassNotFoundException as the Hadoop & Zookeeper jars set by `general.classpaths` are loaded later in Java by the Accumulo classloader.  While `general.classpaths` should available to users, it should be empty by default and the classpath required for Accumulo should be built in `accumulo-env.sh` before `java` command is called.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103733722
 
    --- Diff: assemble/conf/log4j-service.properties ---
    @@ -13,43 +13,47 @@
     # See the License for the specific language governing permissions and
     # limitations under the License.
     
    -## Log4j 1.2 file that configures logging for all Accumulo services (Master, TabletServer, GC & Tracer) except Monitor
    +## Log4j 1.2 file that configures logging for all Accumulo services (Master, TabletServer, GC, and Tracer) except Monitor
     ## The system properties referenced below are configured by accumulo-env.sh
     
    -## Write out INFO and higher to log file
    +## Define a log file appender
     log4j.appender.file=org.apache.log4j.RollingFileAppender
    -log4j.appender.file.File=${accumulo.log.dir}/${accumulo.service.id}.log
    +log4j.appender.file.File=${accumulo.log.dir}/${accumulo.application}.log
     log4j.appender.file.MaxFileSize=100MB
     log4j.appender.file.MaxBackupIndex=10
     log4j.appender.file.Threshold=INFO
     log4j.appender.file.layout=org.apache.log4j.PatternLayout
     log4j.appender.file.layout.ConversionPattern=%d{ISO8601} [%-8c{2}] %-5p: %m%n
     
    -## Forwards Accumulo messages at WARN or higher to Accumulo Monitor
    -log4j.appender.monitor=org.apache.accumulo.start.util.AsyncSocketAppender
    -log4j.appender.monitor.RemoteHost=${accumulo.monitor.host}
    -log4j.appender.monitor.Port=${accumulo.monitor.log.port}
    -log4j.appender.monitor.Application=${accumulo.service.id}
    +## Define an appender to send important logs to the the primary Accumulo Monitor
    +## The primary monitor is the one currently holding a shared lock in ZooKeeper,
    +## and is typically the one that started first.
    +log4j.appender.monitor=org.apache.accumulo.monitor.util.AccumuloMonitorAppender
     log4j.appender.monitor.Threshold=WARN
     
    -## Configures Audit logs which are OFF by default.
    +## Uncomment to define a log file appender for audit logs
     #log4j.appender.audit=org.apache.log4j.DailyRollingFileAppender
    -#log4j.appender.audit.File=${accumulo.log.dir}/${accumulo.audit.log}
    +#log4j.appender.audit.File=${accumulo.log.dir}/${accumulo.application}.audit
    --- End diff --
   
    Yeah, it's just an example, though. I was mostly trying to minimize the number of system properties we were creating and using for the log4j config. We've already changed the name of the audit named logger, so its already a change in behavior. I think separating things out will help users track access to the various services. At this point, we've made it so users can easily do something different if they wish.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103733871
 
    --- Diff: assemble/conf/templates/accumulo-env.sh ---
    @@ -53,20 +58,37 @@ shell)   JAVA_OPTS=("${JAVA_OPTS[@]}" ${shellHigh_shellLow}) ;;
     *)       JAVA_OPTS=("${JAVA_OPTS[@]}" ${otherHigh_otherLow}) ;;
     esac
     
    -## JVM options set for logging.  Review logj4 properties files to see how they are used.
    -JAVA_OPTS=("${JAVA_OPTS[@]}"
    +## JVM options set for logging. Review logj4 properties files to see how they are used.
    +JAVA_OPTS=("${JAVA_OPTS[@]}"
       "-Daccumulo.log.dir=${ACCUMULO_LOG_DIR}"
    -  "-Daccumulo.service.id=${ACCUMULO_CMD}${ACCUMULO_SERVICE_INSTANCE}_$(hostname)"
    -  "-Daccumulo.audit.log=$(hostname).audit")
    +  "-Daccumulo.application=${ACCUMULO_CMD}${ACCUMULO_SERVICE_INSTANCE}_$(hostname)")
     
     case "$ACCUMULO_CMD" in
    -monitor)                    JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=file:${ACCUMULO_CONF_DIR}/log4j-monitor.properties") ;;
    -gc|master|tserver|tracer)   JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=file:${ACCUMULO_CONF_DIR}/log4j-service.properties") ;;
    -*)                          JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=file:${ACCUMULO_CONF_DIR}/log4j.properties") ;;
    +  monitor)
    +    JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=log4j-monitor.properties")
    +    ;;
    +  gc|master|tserver|tracer)
    +    JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=log4j-service.properties")
    +    ;;
    +  *)
    +    # let log4j use its default behavior (log4j.xml, log4j.properties)
    +    true
    +    ;;
     esac
     
     export JAVA_OPTS
     
    +## External class path items for Java system class loader (dependencies not included with Accumulo)
    --- End diff --
   
    Agreed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103734610
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/util/AccumuloMonitorAppender.java ---
    @@ -0,0 +1,153 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.monitor.util;
    +
    +import static java.nio.charset.StandardCharsets.UTF_8;
    +
    +import java.util.concurrent.Executors;
    +import java.util.concurrent.ScheduledExecutorService;
    +import java.util.concurrent.TimeUnit;
    +import java.util.concurrent.atomic.AtomicBoolean;
    +
    +import org.apache.accumulo.core.Constants;
    +import org.apache.accumulo.core.client.Instance;
    +import org.apache.accumulo.core.conf.Property;
    +import org.apache.accumulo.core.zookeeper.ZooUtil;
    +import org.apache.accumulo.fate.zookeeper.ZooCache;
    +import org.apache.accumulo.fate.zookeeper.ZooCacheFactory;
    +import org.apache.accumulo.server.client.HdfsZooInstance;
    +import org.apache.log4j.AsyncAppender;
    +import org.apache.log4j.net.SocketAppender;
    +import org.apache.zookeeper.data.Stat;
    +
    +import com.google.common.net.HostAndPort;
    +
    +public class AccumuloMonitorAppender extends AsyncAppender {
    --- End diff --
   
    Yes, I created a thread in the master which logged a message every 10 seconds, and killed and restarted the master on a few different ports. It's how I found that I needed to check for a change in the `modified zookeeper transaction id` instead of just a change in the value... otherwise it wouldn't update if the monitor was restarted at the same location.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103735127
 
    --- Diff: assemble/bin/accumulo ---
    @@ -127,13 +127,12 @@ function main() {
         JAVA=($ACCUMULO_JAVA_PREFIX $JAVA)
       fi
       
    -  CLASSPATH="${lib}/accumulo-start.jar:${conf}:${lib}/slf4j-api.jar:${lib}/slf4j-log4j12.jar:${lib}/log4j.jar:${CLASSPATH}"
     
    -  exec "${JAVA[@]}" "-Dapp=$ACCUMULO_CMD" \
    +  CLASSPATH="${conf}:${lib}/*:${CLASSPATH}"
    --- End diff --
   
    The common case would be to have this be non-null, since the rest of the class path will likely be set in the env file. This keeps it simple and readable, and extra colons in the class path don't cause issues.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103736418
 
    --- Diff: assemble/conf/templates/accumulo-env.sh ---
    @@ -53,20 +58,37 @@ shell)   JAVA_OPTS=("${JAVA_OPTS[@]}" ${shellHigh_shellLow}) ;;
     *)       JAVA_OPTS=("${JAVA_OPTS[@]}" ${otherHigh_otherLow}) ;;
     esac
     
    -## JVM options set for logging.  Review logj4 properties files to see how they are used.
    -JAVA_OPTS=("${JAVA_OPTS[@]}"
    +## JVM options set for logging. Review logj4 properties files to see how they are used.
    +JAVA_OPTS=("${JAVA_OPTS[@]}"
       "-Daccumulo.log.dir=${ACCUMULO_LOG_DIR}"
    -  "-Daccumulo.service.id=${ACCUMULO_CMD}${ACCUMULO_SERVICE_INSTANCE}_$(hostname)"
    -  "-Daccumulo.audit.log=$(hostname).audit")
    +  "-Daccumulo.application=${ACCUMULO_CMD}${ACCUMULO_SERVICE_INSTANCE}_$(hostname)")
     
     case "$ACCUMULO_CMD" in
    -monitor)                    JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=file:${ACCUMULO_CONF_DIR}/log4j-monitor.properties") ;;
    -gc|master|tserver|tracer)   JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=file:${ACCUMULO_CONF_DIR}/log4j-service.properties") ;;
    -*)                          JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=file:${ACCUMULO_CONF_DIR}/log4j.properties") ;;
    +  monitor)
    +    JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=log4j-monitor.properties")
    +    ;;
    +  gc|master|tserver|tracer)
    +    JAVA_OPTS=("${JAVA_OPTS[@]}" "-Dlog4j.configuration=log4j-service.properties")
    +    ;;
    +  *)
    +    # let log4j use its default behavior (log4j.xml, log4j.properties)
    +    true
    +    ;;
     esac
     
     export JAVA_OPTS
     
    +## External class path items for Java system class loader (dependencies not included with Accumulo)
    +CLASSPATH="$(find "$ZOOKEEPER_HOME"/{,lib} "$HADOOP_PREFIX"/share/hadoop/{common,common/lib,hdfs,mapreduce,yarn} -maxdepth 1 -name '*.jar' \
    +  -and -not -name '*slf4j*' \
    +  -and -not -name '*fatjar*' \
    +  -and -not -name '*-javadoc*' \
    +  -and -not -name '*-sources*.jar' \
    +  -and -not -name '*-test*.jar' \
    +  -print0 | tr '\0' ':')$CLASSPATH"
    --- End diff --
   
    It was included in the second commit, where I switch to use the new appender, because the use of this appender requires some of these classes to be available during start-up.
   
    It's also something I've previously discussed doing as a sensible default: using standard java class path solutions in our launch scripts for bootstrap, rather than the xml configs with our custom class loading. This is easier for users to configure, more consistent with standard java applications, and easier for downstream packaging.
   
    I'd like to make further changes in this area, in issues more explicitly focused on that, but I needed to a little bit here, as a bare minimum, to get this new appender to work as a default.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #223: ACCUMULO-4409 Create AccumuloMonitorAppender

joshelser
In reply to this post by joshelser
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/223#discussion_r103737450
 
    --- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/util/AccumuloMonitorAppender.java ---
    @@ -0,0 +1,153 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.monitor.util;
    +
    +import static java.nio.charset.StandardCharsets.UTF_8;
    +
    +import java.util.concurrent.Executors;
    +import java.util.concurrent.ScheduledExecutorService;
    +import java.util.concurrent.TimeUnit;
    +import java.util.concurrent.atomic.AtomicBoolean;
    +
    +import org.apache.accumulo.core.Constants;
    +import org.apache.accumulo.core.client.Instance;
    +import org.apache.accumulo.core.conf.Property;
    +import org.apache.accumulo.core.zookeeper.ZooUtil;
    +import org.apache.accumulo.fate.zookeeper.ZooCache;
    +import org.apache.accumulo.fate.zookeeper.ZooCacheFactory;
    +import org.apache.accumulo.server.client.HdfsZooInstance;
    +import org.apache.log4j.AsyncAppender;
    +import org.apache.log4j.net.SocketAppender;
    +import org.apache.zookeeper.data.Stat;
    +
    +import com.google.common.net.HostAndPort;
    +
    +public class AccumuloMonitorAppender extends AsyncAppender {
    +
    +  private final ScheduledExecutorService executorService;
    +  private final AtomicBoolean trackerScheduled;
    +
    +  /**
    +   * A Log4j Appender which follows the registered location of the active Accumulo monitor service, and forwards log messages to it
    +   */
    +  public AccumuloMonitorAppender() {
    +    // create the background thread to watch for updates to monitor location
    +    trackerScheduled = new AtomicBoolean(false);
    +    executorService = Executors.newSingleThreadScheduledExecutor(runnable -> {
    +      Thread t = new Thread(runnable, AccumuloMonitorAppender.class.getSimpleName() + " Location Tracker");
    +      t.setDaemon(true);
    +      return t;
    --- End diff --
   
    Yes, we have a Daemon subclass of Thread, which is a trivial wrapper. I didn't feel the need to use it because I think it's better that developers can read this code without any knowledge or understanding of the behavior of some custom class.
   
    I'd probably favor deleting our Daemon class. I'm skeptical that it's all that useful.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
12
Loading...