> On Feb. 8, 2014, 12:48 a.m., kturner wrote: > > server/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java, > > line 95 > > <https://reviews.apache.org/r/17860/diff/1/?file=480379#file480379line95> > > > > If getData() throws NoNodeException (or returns null? not sure if that > > will happen) should probably gracefully handle this and stop trying to send > > log data wherever it used to be sending it. > > Josh Elser wrote: > This was another direct copy/paste from what was previously there. Is > there a race condition from a node going away that you have a Watcher set on? > The initialization does check that the path exists before initially > configuring log4j. > > When the monitor goes away (and "quiteMode" is turned on the logger) we > just ignore the errors about not being able to talk to the remote and then > the messages will eventually get dropped due to the configuration. This is > the same thing as what presently happens. I have no idea if we would have any > noticeable positive impact to disabling that appender (or similar) when we > see that the logger goes away rather than just letting the framework roll > away those messages. > > kturner wrote: > I am not thinking of a race condition, just thinking of the condition > where the node is there and then later goes away. If it were an ephemeral > node, it would go away when the monitor processes died. > > Zookeeper will call the watcher when the node goes away. Then because > exist is called, the watcher will be called if it ever comes back. > >
So are you suggesting to just check for NodeDeleted as the type in process(WatchedEvent) before trying to pull the data out of the node? Fail-fast and (potentially) turn off logging? - Josh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17860/#review33988 ----------------------------------------------------------- On Feb. 7, 2014, 10:30 p.m., Josh Elser wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17860/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2014, 10:30 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-2334 > https://issues.apache.org/jira/browse/ACCUMULO-2334 > > > Repository: accumulo > > > Description > ------- > > Removes the necessity to compute and set ACCUMULO_LOG_HOST. We can accomplish > this by having the monitor advertise a host in addition to just the port and > have the other processes place a watcher on that node in zookeeper. > > This has some nice benefits to simplifying the code, removing the annoying > (IMO) ACCUMULO_LOG_HOST variable, and, when running the monitor on a random > port, the other services can auto-discover without having to update > configuration files and restart the services. The auto-discovery also has > benefit if the monitor has to be moved to a different host (or multiple > monitors are being run with "failover") > > > Diffs > ----- > > conf/examples/1GB/native-standalone/accumulo-env.sh aa4a1d0 > conf/examples/1GB/standalone/accumulo-env.sh 1707f3d > conf/examples/2GB/native-standalone/accumulo-env.sh ef74ca7 > conf/examples/2GB/standalone/accumulo-env.sh 75014c2 > conf/examples/3GB/native-standalone/accumulo-env.sh ae0da11 > conf/examples/3GB/standalone/accumulo-env.sh 7edd938 > conf/examples/512MB/native-standalone/accumulo-env.sh 749a678 > conf/examples/512MB/standalone/accumulo-env.sh 9beb059 > core/src/main/java/org/apache/accumulo/core/Constants.java 9bb3419 > core/src/main/java/org/apache/accumulo/core/util/AddressUtil.java af9a1a6 > server/src/main/java/org/apache/accumulo/server/Accumulo.java f4da33b > server/src/main/java/org/apache/accumulo/server/monitor/LogService.java > 10ef9e4 > server/src/main/java/org/apache/accumulo/server/monitor/Monitor.java > dcb80fd > > server/src/main/java/org/apache/accumulo/server/watcher/MonitorLog4jWatcher.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/17860/diff/ > > > Testing > ------- > > Local testing so far. Configure the monitor to use a random log4j port, start > an instance. Then, test log forwarding works. Restart the monitor, check that > the tserver saw the update, and that log forwarding still works even though > the monitor is now listening on a different port for log4j. > > > Thanks, > > Josh Elser > >
