Fixes #842 Support multiple ways for running Fluo applications #883

mikewalch · 2017-07-17T19:16:44Z

Split fluo.properties into connection.properties and
application.properties
Renamed several properties to support split
Support loading FluoConfiguration from multiple files
Created main methods commands: FluoOracle, FluoWorker,
FluoGetJars, FluoSetup, FluoList, FluoWait, FluoScan
Updated documentation to reflect changes

mikewalch · 2017-07-17T19:22:45Z

When reviewing this pull request, take a look at the following repos & pull requests:

keith-turner · 2017-07-19T14:39:37Z

modules/distribution/src/main/scripts/fluo

  ;;
 list)
-  check_hadoop
-  java org.apache.fluo.cluster.command.FluoCommand $FLUO_HOME $HADOOP_PREFIX $1 app ${*:2}
+  if [ -f "$conf/connection.properties" ]; then


Instead of checking for non-existence, could check that connection.properties has no props set. This would allow old code that only modifies fluo.props to work w/o having to delete this file.

In f89f6eb, I changed the name of fluo.properties to fluo.properties.deprecated. If users want to use the old config file, they just need to change the name. Existence of both fluo.properties and connection.properties will cause an error in scripts.

keith-turner · 2017-07-19T14:54:33Z

modules/distribution/src/main/lib/fetch.sh

@@ -57,44 +57,44 @@ extra)
  download aopalliance:aopalliance:jar:1.0
  download ch.qos.logback:logback-classic:jar:1.1.3 ./logback
  download ch.qos.logback:logback-core:jar:1.1.3 ./logback
-  download com.101tec:zkclient:jar:0.3
+  download com.101tec:zkclient:jar:0.3 ./twill


Could group all of these twill jars together in the file

Fixed in f89f6eb

keith-turner · 2017-07-21T21:45:25Z

docs/install.md

+    | [connection.properties]     | Configures connection to Fluo. Required for all commands.                |
+    | [application.properties]    | Configuration passed to `fluo setup` when initializing Fluo application. |
+    | [log4j.properties]          | Configures logging                                                       |
+    | [fluo.properties]           | Deprecated Fluo configuration file                                       |


Could mention what replaces it.

Fixed in da8968e

keith-turner · 2017-07-21T21:46:14Z

docs/install.md


-## Start your application
+        cp conf/application.properties conf/myapp.properties


Not sure if we should advocate copying to conf.

keith-turner · 2017-07-21T21:52:34Z

modules/api/src/main/java/org/apache/fluo/api/config/FluoConfiguration.java

+   *
+   * @param observerJarsUrl URL to observer jars directory
+   */
+  public FluoConfiguration setObserverJarsUrl(String observerJarsUrl) {


keith-turner · 2017-07-21T21:52:52Z

modules/api/src/main/java/org/apache/fluo/api/config/FluoConfiguration.java

  public String getAccumuloClasspath() {
    return getString(ADMIN_ACCUMULO_CLASSPATH_PROP, ADMIN_ACCUMULO_CLASSPATH_DEFAULT);
  }

+  public FluoConfiguration setAccumuloJars(String path) {


keith-turner · 2017-07-21T21:53:50Z

modules/api/src/main/java/org/apache/fluo/api/config/SimpleConfiguration.java

@@ -186,6 +166,30 @@ public String getString(String key, String defaultValue) {
    return internalConfig.getString(key, defaultValue);
  }

+  public void load(InputStream in) {


since tag? javadoc? need to document if the load merges or clears before loading. Also need to document what the behavior is if multiple loads have the same prop, does the last one loaded always win?

keith-turner · 2017-07-21T21:57:13Z

modules/api/src/test/resources/application.properties

+## Accumulo properties
+## -------------------
+## Accumulo instance to connect to
+fluo.accumulo.instance=myInstance


I was surprised to see the accumulo props here instead of connection props. I had expected to see them in connections props. But I don't have a good reason for where they should go. I need to work out the pros and cons of each location.

With #893 I think storing the accumulo credentials in ZK is a good way to go

keith-turner · 2017-07-21T21:58:04Z

modules/api/src/test/resources/fluo.properties

+# 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.
+


Should document that this file is deprecated and only used if conn.props is not present.

Fixed in 241f237

keith-turner · 2017-07-21T22:03:50Z

modules/distribution/src/main/config/fluo-env.sh

@@ -47,7 +74,12 @@ setupClasspathFromSystem()
  test -z "$ACCUMULO_HOME" && ACCUMULO_HOME=/path/to/accumulo
  test -z "$ZOOKEEPER_HOME" && ZOOKEEPER_HOME=/path/to/zookeeper

-  CLASSPATH="$FLUO_HOME/lib/*:$FLUO_HOME/lib/logback/*"
+  CLASSPATH="$lib/*"
+  if [ -f "$conf/connection.properties" ]; then


this needs some comments explaining why it exists

fixed in d464bd9

keith-turner · 2017-07-21T22:06:33Z

modules/distribution/src/main/scripts/fluo

+  echo "  wait <app>                  Waits until all notifications are processed for <app>"
+  echo "  exec <app> <class> {<arg>}  Executes <class> with <args> using classpath for <app>";
+
+  echo -e "\nDeprecated commands:\n"
  echo "  new <app>     Creates configuration for new application in apps/"


should probably annotate deprecated commands in this output

fixed in d464bd9

keith-turner · 2017-07-21T22:08:28Z

modules/command/src/main/java/org/apache/fluo/command/FluoScan.java

+import org.apache.log4j.Level;
+import org.apache.log4j.Logger;
+
+public class FluoScan {


Seems like this should share code with the deprecated scan command

keith-turner · 2017-07-21T22:09:29Z

Is there a command to print the props stored in zookeeper?

keith-turner · 2017-07-25T18:08:55Z

modules/distribution/src/main/scripts/fluo

+  echo "  classpath                   Prints the classpath setup in fluo-env.sh"
+  echo "  list                        Lists all Fluo applications in Fluo instance"
+  echo "  scan <app>                  Prints snapshot of data in Fluo <app>"
+  echo "  setup <appProps> {<arg>}    Sets up Fluo application using <appProps>. Run with -h to see additional args."


I tried running the following. Would it make sense to not require the props file for -h?

$ fluo setup -h Application properties file '-h' does not exist

This is fixed now.. Command is now init rather than setup

keith-turner · 2017-07-25T18:13:16Z

modules/distribution/src/main/config/application.properties

+## Application properties
+## ----------------------
+## Fluo application name
+fluo.connection.application.name=


This could be fluo.application.name

Properties cannot be named starting with fluo.app due to prefix being used for user-specific properties

keith-turner · 2017-07-25T18:16:35Z

modules/distribution/src/main/config/application.properties

+#fluo.observer.provider=com.foo.AppObserverProvider
+## Observer jars in this directory are stored away in HDFS during initialization
+#fluo.observer.init.dir=/path/to/observer/jars/
+## Properties with a prefix of fluo.app.* are stored in zookeeper at


Now everything is stored in zookeeper.

keith-turner · 2017-07-25T18:25:28Z

modules/distribution/src/main/scripts/local-fluo

-. "$bin"/impl/config.sh
+if [ -f "$conf/fluo-env.sh" ]; then
+  . "$conf"/fluo-env.sh
+fi


The help for script should print something about being deprecated.

keith-turner · 2017-07-25T19:57:20Z

modules/distribution/src/main/config/connection.properties

+## Zookeeper connection string specifying host and chroot where Fluo stores data.
+## A chroot directory suffix must be specified but doesn't need to be named
+## '/fluo'.  If not specified, a Fluo application cannot be initialized.
+#fluo.connection.zookeepers=localhost/fluo


What was the reason for changing the prefix on these props from fluo.client?

FluoFactory can create many other things (not just FluoClient)...

https://static.javadoc.io/org.apache.fluo/fluo-api/1.1.0-incubating/org/apache/fluo/api/client/FluoFactory.html

* Split fluo.properties into connection.properties and application.properties * Renamed several properties to support split * Support loading FluoConfiguration from multiple files * Created main methods commands: FluoOracle, FluoWorker, FluoGetJars, FluoSetup, FluoList, FluoWait, FluoScan * Updated documentation to reflect changes

* Deprecated all classes in fluo-cluster module

mikewalch · 2017-07-27T22:13:09Z

Is there a command to print the props stored in zookeeper?

Add 'fluo config' command in 96d1fbe

keith-turner · 2017-07-28T15:07:07Z

modules/api/src/main/java/org/apache/fluo/api/client/FluoAdmin.java

@@ -134,6 +132,8 @@ void initialize(InitializationOptions opts) throws AlreadyInitializedException,
   */
  void updateSharedConfig();

+  FluoConfiguration getSharedConfig();


Needs javadoc and since tag

Could have two methods here. One that returns exactly what config was passed to the factory and another that returns what config is stored in zookeeper. We could create another issue to support merging one simple config into another. This would be a good starter issue.

I created #894

keith-turner · 2017-07-31T20:53:24Z

docs/install.md

+    |------------------------------|----------------------------------------------------------------------------------------------|
+    | [fluo-env.sh]                | Configures classpath for `fluo` script. Required for all commands.                           |
+    | [fluo-conn.properties]       | Configures connection to Fluo. Required for all commands.                                    |
+    | [fluo-app.properties]        | Configuration passed to `fluo init` when initializing Fluo application.                      |


Could mention that this is a template for the file that will be passed to init

keith-turner · 2017-07-31T20:54:37Z

docs/install.md

@@ -50,24 +50,21 @@ After you obtain a Fluo distribution tarball, follow these steps to install Fluo
 1.  Choose a directory with plenty of space and untar the distribution:

        tar -xvzf fluo-1.1.0-incubating-bin.tar.gz
+        cd fluo-1.1.0-incubating


At the very beginning of this file YARN is mentioned, seems like it should not be

keith-turner · 2017-07-31T21:04:58Z

docs/install.md

-be in the `worker_*.log` file for each of your worker containers. 
+   The commands will retrieve your application configuration and observer jars (using your
+   application name) before starting the oracle or worker process. The commands are designed
+   to be run on a cluster to distribute the oracle and workers across multiple nodes.


I think the wording of this is a bit confusing. For the phrase distribute the oracle and workers across multiple nodes what is being distributed? The Fluo software? May be more clear to write After installing the Fluo software on each node, .... Below it mentions installing software on each node, so maybe just the order the information is introduced needs to be reworked?

keith-turner · 2017-07-31T22:37:36Z

docs/install.md

-your YARN configuration `yarn-site.xml` (see [yarn-default.xml] for more info about this property).
-If you are running Fluo on single machine, this method works well. If you are running Fluo on a
-cluster, your containers and their logs will be spread across the cluster.
+The oracle & worker logs can be found in the `logs/` directory of your Fluo installation.


I think the log files are currently prefixed with the app name. I was thinking we could put the logs in a dir that uses the app name instead. A dir like logs/apps/<app name>/ or logs/<app name> . Could do this as a follow on issue.

keith-turner · 2017-07-31T23:17:36Z

modules/command/src/main/java/org/apache/fluo/command/FluoExec.java

+    FluoConfiguration fluoConfig = new FluoConfiguration(connectionPropsFile);
+    fluoConfig.setApplicationName(applicationName);
+    CommandUtil.verifyAppInitialized(fluoConfig);
+    fluoConfig = FluoAdminImpl.mergeZookeeperConfig(fluoConfig);


Not sure this merge should be done. I think it would be less confusing to inject connections props + app name, the minimum needed to create a client.

I did the merge because there are classes (specifically OptimizeTable used by phrasecount) that are written for the fluo exec command and depend on the FluoConfiguration that is passed in to have all configuration set. If we don't do the merge, we need to update these classes to grab all configuration from Zookeeper.

Ok, I understand why the merge is needed now, trying to maintain the behavior of passing the $FLUO_HOME/apps/app/conf/fluo.properties file. This is kinda unfortunate because it means we now have to connect to zookeeper before running the user code.

All that is advertised in the docs is that this config can be used to create a client. I think it would be nice for the old config file to just pass it through and for the new config file only set the app name.

keith-turner · 2017-07-31T23:31:55Z

Need to do some sort of upgrade testing. This could be a follow on task/issue.

mikewalch · 2017-08-01T16:22:24Z

As for upgrade testing, I was able to run phrasecount (using master branch) using a Fluo tarball produced PR. I had to delete fluo-conn.properties (as expected) but everything else worked out of the box.

keith-turner · 2017-08-01T23:06:24Z

modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java

+      }
+
+      if (!accumuloClasspath.isEmpty()) {
+        String contextName = "fluo-" + config.getApplicationName();


This is very nice. I wonder if we need to be careful and check what characters are in the app name

keith-turner · 2017-08-01T23:18:08Z

modules/api/src/main/java/org/apache/fluo/api/config/FluoConfiguration.java

+  /**
+   * @since 1.2.0
+   */
+  public String getHdfsRoot() {


I think it would be nice to drop the H and name the method getDfsRoot(). Currently we only support Hadoop, but in the future we could support URIs for other dfs types.

keith-turner · 2017-08-01T23:19:08Z

modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java

+      try {
+        fs.copyFromLocalFile(new Path(jarPath), new Path(hdfsDestDir));
+      } catch (IOException e) {
+        logger.error("Failed to copy file {} to HDFS directory {}", jarPath, hdfsDestDir);


Why not throw an exception when this fails?

keith-turner · 2017-08-01T23:21:58Z

modules/distribution/src/main/config/fluo-app.properties

+## implements org.apache.fluo.api.observer.ObserverProvider.
+#fluo.observer.provider=com.foo.AppObserverProvider
+## Observer jars in this directory are stored away in HDFS during initialization
+#fluo.observer.init.dir=/path/to/observer/jars/


In addition to this prop, it seems like the new accumulo jars prop should be documented somewhere in this file.

keith-turner · 2017-08-01T23:25:33Z

modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java

+
+      String accumuloClasspath;
+      if (!accumuloJars.isEmpty()) {
+        accumuloClasspath = copyJarsToHdfs(accumuloJars, "lib/accumulo");


This is a nice change, putting the accumulo jars in a per app dfs dir.

keith-turner · 2017-08-02T01:24:43Z

modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java

        conn.tableOperations().setProperty(config.getAccumuloTable(),
            AccumuloProps.TABLE_CLASSPATH, contextName);
      }

+      if (config.getObserverJarsUrl().isEmpty() && !config.getObserverInitDir().trim().isEmpty()) {


Are there any test for diff combos of these configs? Combos of accumulo jars, init dir, observers jars, etc

I added check to FluoAdminImpl.initialized()

keith-turner · 2017-08-02T01:29:36Z

modules/core/src/main/java/org/apache/fluo/core/client/FluoClientImpl.java

+    Objects.requireNonNull(connConfig);
+    Preconditions.checkArgument(connConfig.hasRequiredConnectionProps(),
+        "missing required connection properties");
+    config = FluoAdminImpl.mergeZookeeperConfig(connConfig);


I think this call creates a curator and closes it. I suspect later in this method a curator is created, could possibly refactor code to avoid creating multiple connections to ZK.

Created #896

keith-turner · 2017-08-02T01:30:55Z

modules/core/src/main/java/org/apache/fluo/core/impl/Environment.java

-        config.setProperty(prop, sharedProps.getProperty(prop));
-        tmpConfig.setProperty(prop, sharedProps.getProperty(prop));
-      }
+      config = FluoAdminImpl.mergeZookeeperConfig(config);


could curator be passed here?

Created #896

keith-turner · 2017-08-02T01:39:12Z

modules/distribution/src/main/scripts/fluo

-  echo "  wait <app>    Waits until all notifications are processed"
-  echo "  version       Prints the version of Fluo"
-  echo "  exec <app> <class>  {<argument>}";
+  echo "  init <appProps> {<arg>}     Initializes Fluo application using <appProps>. Run with -h to see additional args."


I had a thought, not sure if it will work. If this were of the form init <app name> <appProps> {args} then it would be more consistent with other commands and the fluo-app.props file would not require the name prop.

keith-turner · 2017-08-02T01:46:42Z

modules/distribution/src/main/config/fluo-env.sh

+
+export FLUO_LOG_ID="${cmd}_$(hostname)_$(date +%s)"
+
+JAVA_OPTS=("-Dlog4j.configuration=file:${FLUO_LOG4J_CONFIG}"


It seems like these JAVA_OPTS are only used for services? If so, I think the name should changes. At first I thought maybe all of the commands would use this (like fluo init). Also, if all of the logging stuff is only used for services it would be nice to group all of the service config into a section (delimited by the ############)

Fixed.. changed JAVA_OPTS to SERVICE_OPTS

keith-turner · 2017-08-02T01:57:34Z

modules/api/src/test/java/org/apache/fluo/api/config/FluoConfigurationTest.java

@@ -41,14 +41,14 @@

  @Test
  public void testDefaults() {
-    Assert.assertEquals(FluoConfiguration.CLIENT_ZOOKEEPER_CONNECT_DEFAULT,
+    Assert.assertEquals(FluoConfiguration.CONNECTION_ZOOKEEPERS_DEFAULT,


Did you try running this test with code coverage? If not I can try that.

No I didn't

keith-turner · 2017-08-02T16:22:51Z

@mikewalch re upgrade testing. I am thinking of initializing and starting an app with 1.0 or 1.1, stopping the app, and then trying to start it with 1.2.0-SNAPSHOT.

keith-turner · 2017-08-03T13:58:36Z

modules/distribution/src/main/config/fluo-env.sh

-##################################################################
+#####################################################################
+# Build SERVICE_OPTS variable. Defaults below work but can be edited.
+#####################################################################


Should all of the logging stuff move to this new section?

Fixed in bebb493. Let me know what you think.

Made this change online using the github editor

keith-turner reviewed Jul 19, 2017

View reviewed changes

keith-turner reviewed Jul 21, 2017

View reviewed changes

mikewalch force-pushed the fluo-842 branch from 3b6c851 to f89f6eb Compare July 25, 2017 19:46

keith-turner reviewed Jul 25, 2017

View reviewed changes

mikewalch added 12 commits July 27, 2017 18:09

CR updates

54639f4

Documentation update and fixed help for 'fluo init'

1b82bdb

Improved config file names

5ade6d7

Improved command usage

03b0a3a

Added since tags and javadoc

915f22b

Added more since tags

2495488

Renamed file

b221f68

whitespace formatting

ba3d785

CR update

530943b

Moved shared scanning code to ScanUtil

7c656c5

* Deprecated all classes in fluo-cluster module

Added 'fluo config' command

96d1fbe

mikewalch force-pushed the fluo-842 branch from ee679e7 to 96d1fbe Compare July 27, 2017 22:11

keith-turner reviewed Jul 28, 2017

View reviewed changes

keith-turner mentioned this pull request Jul 28, 2017

Support locking down properties in zookeeper #893

Closed

CR updates

549988d

keith-turner reviewed Jul 31, 2017

View reviewed changes

mikewalch added 2 commits August 1, 2017 12:26

CR updates

241f237

CR updates

d464bd9

keith-turner reviewed Aug 2, 2017

View reviewed changes

mikewalch added 7 commits August 2, 2017 15:26

CR updates

a3ed356

More updates

3fdf5ac

Add since tag

ffe2922

Using configurable pattern

7f3bbd8

formatting

72373bc

Fixed javadoc

93d3317

CR updates

98e2262

mikewalch mentioned this pull request Aug 3, 2017

Improve 'fluo list' to print number of running oracles and workers #895

Closed

keith-turner reviewed Aug 3, 2017

View reviewed changes

mikewalch and others added 2 commits August 3, 2017 13:31

CR updates

bebb493

Update FluoExec.java

f23866e

Made this change online using the github editor

keith-turner approved these changes Aug 4, 2017

View reviewed changes

Adding back mergeZookeperConfig to FluoExec

c0e0a11

mikewalch mentioned this pull request Aug 4, 2017

Determine if FluoExec should read config from Zookeeper #898

Open

mikewalch merged commit cf608fe into apache:master Aug 4, 2017

mikewalch deleted the fluo-842 branch August 4, 2017 15:19

This was referenced Oct 17, 2017

Create FluoYarnLauncher #853

Closed

Support loading FluoConfiguration from multiple files #844

Closed

mikewalch mentioned this pull request Nov 6, 2017

Investigate Mesos as an option for running Fluo #290

Closed


		## Start your application
		cp conf/application.properties conf/myapp.properties


		export FLUO_LOG_ID="${cmd}_$(hostname)_$(date +%s)"

		JAVA_OPTS=("-Dlog4j.configuration=file:${FLUO_LOG4J_CONFIG}"

Fixes #842 Support multiple ways for running Fluo applications #883

Fixes #842 Support multiple ways for running Fluo applications #883

Conversation

mikewalch commented Jul 17, 2017

mikewalch commented Jul 17, 2017

keith-turner Jul 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikewalch Jul 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith-turner commented Jul 21, 2017

Choose a reason for hiding this comment

mikewalch Jul 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikewalch commented Jul 27, 2017

Choose a reason for hiding this comment

keith-turner Jul 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith-turner commented Jul 31, 2017

mikewalch commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith-turner Jul 19, 2017 •

edited

mikewalch Jul 20, 2017 •

edited

mikewalch Jul 26, 2017 •

edited

keith-turner Jul 28, 2017 •

edited