> On Feb. 10, 2014, 11:24 p.m., kturner wrote: > > server/src/main/java/org/apache/accumulo/server/monitor/Monitor.java, line > > 554 > > <https://reviews.apache.org/r/17860/diff/2-3/?file=481200#file481200line554> > > > > Have you manually tested upgrade? > > > > It may be better to put this w/ the zookeeper upgrade code in Master. > > Would make it easier when someones working on 1.7 upgrade to be aware of it > > and possibly remove it. > > Josh Elser wrote: > Yes. I tested it against a 1.5.1-SNAPSHOT install I had locally that did > not yet have these changes to ZooKeeper and it worked just fine. > > I can move it over to the Master code. I'll look at doing that to make > sure it makes sense. > > kturner wrote: > there is a function called upgradeZookeeper() in Master > > Josh Elser wrote: > I don't think it makes sense to move the code into the Master. As it > stands now, we can trigger the zookeeper check when the monitor starts up. If > we move it to the monitor, we'd have to wait for the Master to come up and > finish the upgrade, and then start. I think it's simpler to just leave it in > the Monitor. > > kturner wrote: > I agree w/ what you said. Maybe a better solution is to put all of the > upgrade code in one package or class. Master can call code here and monitor > can call method here to upgrade itself. This would put all of the upgrade > related code in one place making it easier for future upgrade related > changes. Or maybe put a comment in the master upgrade code that mentions > there is also upgrade code in the monitor, so at least there is a pointer to > it. M
SGTM. I'll also make a ticket for 1.7 about consolidating upgrade code in one place (server-base sounds like an intelligent location off the cuff). - Josh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17860/#review34124 ----------------------------------------------------------- On Feb. 10, 2014, 10:26 p.m., Josh Elser wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17860/ > ----------------------------------------------------------- > > (Updated Feb. 10, 2014, 10:26 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/conf/Property.java 424b737 > 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/util/Info.java 7a07107 > server/src/main/java/org/apache/accumulo/server/util/Initialize.java > baa5400 > > 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 > >
