Re: Review Request 35986: Allow slave attributes flag take a value with ':'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/ --- (Updated July 7, 2015, 5:55 p.m.) Review request for mesos, Adam B and Isabel Jimenez. Bugs: MESOS-2868 https://issues.apache.org/jira/browse/MESOS-2868 Repository: mesos Description --- Allow slave attributes flag take a value with ':'. Diffs (updated) - src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 Diff: https://reviews.apache.org/r/35986/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 35986: Allow slave attributes flag take a value with ':'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/ --- (Updated July 7, 2015, 5:46 p.m.) Review request for mesos, Adam B and Isabel Jimenez. Bugs: MESOS-2868 https://issues.apache.org/jira/browse/MESOS-2868 Repository: mesos Description --- Allow slave attributes flag take a value with ':'. Diffs (updated) - src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 Diff: https://reviews.apache.org/r/35986/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 35986: Allow slave attributes flag take a value with ':'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/ --- (Updated July 7, 2015, 7:07 p.m.) Review request for mesos, Adam B and Isabel Jimenez. Bugs: MESOS-2868 https://issues.apache.org/jira/browse/MESOS-2868 Repository: mesos Description --- Allow slave attributes flag take a value with ':'. Diffs (updated) - src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 Diff: https://reviews.apache.org/r/35986/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 35986: Allow slave attributes flag take a value with ':'.
On July 7, 2015, 1:30 a.m., Adam B wrote: src/common/attributes.cpp, lines 150-152 https://reviews.apache.org/r/35986/diff/1/?file=994137#file994137line150 There's a subtle difference in behavior between strings::tokenize and strings::split. For tokenize, Empty tokens will not be included in the result. whereas for split, Empty tokens are allowed in the result. so you need to test not only that `pairs.size() == 2`, but also that `!pairs[0].empty()` and `!pairs[1].empty()`. Let's create unit tests for handling `:foo` and `foo:`. Hi, @adam-mesos, could not add test unit tests for handling :foo and foo:. Because use LOG(FATAL) in parse. - haosdent --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/#review90623 --- On July 7, 2015, 5:55 p.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/ --- (Updated July 7, 2015, 5:55 p.m.) Review request for mesos, Adam B and Isabel Jimenez. Bugs: MESOS-2868 https://issues.apache.org/jira/browse/MESOS-2868 Repository: mesos Description --- Allow slave attributes flag take a value with ':'. Diffs - src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 Diff: https://reviews.apache.org/r/35986/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 35986: Allow slave attributes flag take a value with ':'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/#review90769 --- Patch looks great! Reviews applied: [35986] All tests passed. - Mesos ReviewBot On July 7, 2015, 7:07 p.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/ --- (Updated July 7, 2015, 7:07 p.m.) Review request for mesos, Adam B and Isabel Jimenez. Bugs: MESOS-2868 https://issues.apache.org/jira/browse/MESOS-2868 Repository: mesos Description --- Allow slave attributes flag take a value with ':'. Diffs - src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 Diff: https://reviews.apache.org/r/35986/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 35986: Allow slave attributes flag take a value with ':'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/ --- (Updated July 7, 2015, 6:33 p.m.) Review request for mesos, Adam B and Isabel Jimenez. Changes --- Sorry for so much noisy... Bugs: MESOS-2868 https://issues.apache.org/jira/browse/MESOS-2868 Repository: mesos Description --- Allow slave attributes flag take a value with ':'. Diffs (updated) - src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 Diff: https://reviews.apache.org/r/35986/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 35986: Allow slave attributes flag take a value with ':'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/ --- (Updated July 7, 2015, 6:26 p.m.) Review request for mesos, Adam B and Isabel Jimenez. Bugs: MESOS-2868 https://issues.apache.org/jira/browse/MESOS-2868 Repository: mesos Description --- Allow slave attributes flag take a value with ':'. Diffs (updated) - Makefile.am f8e958d376efa55ace7c5727a31b8e747403641e configure.ac 546c9bbf775a4ef481fafb3a58c85c6d80e19500 docs/ec2-scripts.md PRE-CREATION docs/persistent-volume.md e3dfe6d785bbfda2489ba5d71cad043f290fb23a docs/tools.md 09619f2c162b1765e7718fc314e77b0f77148b97 docs/using-the-mesos-submit-tool.md PRE-CREATION ec2/Makefile.am PRE-CREATION ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/core-site.xml PRE-CREATION ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/hadoop-env.sh PRE-CREATION ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/hdfs-site.xml PRE-CREATION ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/mapred-site.xml PRE-CREATION ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/masters PRE-CREATION ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/slaves PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/cluster-url PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/copy-dir PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/create-swap PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/hadoop-framework-conf/core-site.xml PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/hadoop-framework-conf/hadoop-env.sh PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/hadoop-framework-conf/mapred-site.xml PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/haproxy+apache/haproxy.config.template PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/hypertable/Capfile PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/hypertable/hypertable.cfg PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/masters PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/mesos-daemon PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/redeploy-mesos PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/setup PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/setup-slave PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/setup-torque PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/slaves PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/ssh-no-keychecking PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/start-hypertable PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/start-mesos PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/stop-hypertable PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/stop-mesos PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/zoo PRE-CREATION ec2/deploy.amazon64-old/root/persistent-hdfs/conf/core-site.xml PRE-CREATION ec2/deploy.amazon64-old/root/persistent-hdfs/conf/hadoop-env.sh PRE-CREATION ec2/deploy.amazon64-old/root/persistent-hdfs/conf/hdfs-site.xml PRE-CREATION ec2/deploy.amazon64-old/root/persistent-hdfs/conf/mapred-site.xml PRE-CREATION ec2/deploy.amazon64-old/root/persistent-hdfs/conf/masters PRE-CREATION ec2/deploy.amazon64-old/root/persistent-hdfs/conf/slaves PRE-CREATION ec2/deploy.amazon64-old/root/spark/conf/spark-env.sh PRE-CREATION ec2/deploy.amazon64/root/ephemeral-hdfs/conf/core-site.xml PRE-CREATION ec2/deploy.amazon64/root/ephemeral-hdfs/conf/hadoop-env.sh PRE-CREATION ec2/deploy.amazon64/root/ephemeral-hdfs/conf/hdfs-site.xml PRE-CREATION ec2/deploy.amazon64/root/ephemeral-hdfs/conf/mapred-site.xml PRE-CREATION ec2/deploy.amazon64/root/ephemeral-hdfs/conf/masters PRE-CREATION ec2/deploy.amazon64/root/ephemeral-hdfs/conf/slaves PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/cluster-url PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/copy-dir PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/create-swap PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/hadoop-framework-conf/core-site.xml PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/hadoop-framework-conf/hadoop-env.sh PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/hadoop-framework-conf/mapred-site.xml PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/haproxy+apache/haproxy.config.template PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/hypertable/Capfile PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/hypertable/hypertable.cfg PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/masters PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/mesos-daemon PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/redeploy-mesos PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/setup PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/setup-slave PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/setup-torque PRE-CREATION
Re: Review Request 35986: Allow slave attributes flag take a value with ':'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/#review90623 --- Very good, but there are some subtleties that we should not ignore regarding empty names/values and multiple attributes with colons. src/common/attributes.cpp (lines 150 - 152) https://reviews.apache.org/r/35986/#comment143744 There's a subtle difference in behavior between strings::tokenize and strings::split. For tokenize, Empty tokens will not be included in the result. whereas for split, Empty tokens are allowed in the result. so you need to test not only that `pairs.size() == 2`, but also that `!pairs[0].empty()` and `!pairs[1].empty()`. Let's create unit tests for handling `:foo` and `foo:`. src/tests/attributes_tests.cpp (line 55) https://reviews.apache.org/r/35986/#comment143738 Would love to see this test multiple attributes, so we can be sure that subsequent colons still work as expected in situations like `parse(attr1:foo:bar;attr2:baz:qux)`. - Adam B On June 28, 2015, 6:49 a.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/ --- (Updated June 28, 2015, 6:49 a.m.) Review request for mesos and Isabel Jimenez. Bugs: MESOS-2868 https://issues.apache.org/jira/browse/MESOS-2868 Repository: mesos Description --- Allow slave attributes flag take a value with ':'. Diffs - src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 Diff: https://reviews.apache.org/r/35986/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 35986: Allow slave attributes flag take a value with ':'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/#review89676 --- Patch looks great! Reviews applied: [35986] All tests passed. - Mesos ReviewBot On June 28, 2015, 1:49 p.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/ --- (Updated June 28, 2015, 1:49 p.m.) Review request for mesos and Isabel Jimenez. Bugs: MESOS-2868 https://issues.apache.org/jira/browse/MESOS-2868 Repository: mesos Description --- Allow slave attributes flag take a value with ':'. Diffs - src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 Diff: https://reviews.apache.org/r/35986/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 35986: Allow slave attributes flag take a value with ':'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/#review89695 --- Ship it! Ship It! - Isabel Jimenez On June 28, 2015, 1:49 p.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/ --- (Updated June 28, 2015, 1:49 p.m.) Review request for mesos and Isabel Jimenez. Bugs: MESOS-2868 https://issues.apache.org/jira/browse/MESOS-2868 Repository: mesos Description --- Allow slave attributes flag take a value with ':'. Diffs - src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 Diff: https://reviews.apache.org/r/35986/diff/ Testing --- make check Thanks, haosdent huang
