GitHub user fsareshwala opened a pull request:

https://github.com/apache/bigtop/pull/86

BIGTOP-2317. Add smoke tests for QFS

This change adds some simple testing for qfs running under hadoop. The bigtop
repository already has smoke tests for the hadoop mapreduce framework. It simply
runs the examples through the hadoop infrastructure and ensures that the output
is valid and expected files within the filesystem are present. This change
modifies the bigtop code to support running the same examples but using qfs as
the backend filesystem. This is done through dynamically supplying the hadoop
command to use. In the hadoop hdfs mapreduce scenario, we use the standard
hadoop command that uses hdfs as the backend filesystem. In the hadoop qfs
mapreduce scenario, we use the hadoop-qfs command that uses qfs as the backend
filesystem. The qfs tests simply reference the mapreduce tests and tell it to
use the hadoop-qfs command instead of the hadoop command.

I have tested this works by running the qfs smoke tests using the BigTop
provisioner under docker. The build and tests pass without issue.

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

$ git pull https://github.com/fsareshwala/bigtop qfs-smoke-tests

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

https://github.com/apache/bigtop/pull/86.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 #86

A few comments:

• you don't need to write an essay for a git commit. There's a JIRA ticket, where you can explain anything that has to be explain. So, please limit the commit message to simply "JIRA ###. JIRA title"
• set HADOOP_COMMAND to default hadoop, so you don't need to string it all the way from mapreduce smoke. Which is orthogonal to your change. In general, concerns like this should be separated and isolated, which leads to a way cleaner code.
• same in the TestHadoopExemples: instead of the assert, just initialize that HADOOP_COMMAND constant with default value, to guarantee preserve the original behavior.
• in Groovy you can avoid String concatenation like {{sh.exec(HADOOP_COMMAND + " fs -put $ {source}/. .");}} Instead of you can simply {{sh.exec("${HADOOP_COMMAND} fs -put ${source}

  /. .");}}

Thanks!

Thanks for reviewing the code Cos! I've made the requested changes and updated the pull request. Please take another look when you have a moment.

Errr, I meant: The puppet recipies worked and triggered bugs in packaging: -> new ticket

Thanks Olaf Flebbe. I've put a patch in for BIGTOP-2367.

Does anyone else have any comments regarding this code? Can we merge it?

there seems to be issues with our smoke test infrastructure. Since I am not too familiar with it, I like to iron it with your tests as an example.

Right now I am using this CI script:

# setup configuration file
cat > bigtop-deploy/vm/vagrant-puppet-docker/vagrantconfig_Bigtop-Docker-provisioner-debian-8.yaml <<-__EOT__
docker:
        memory_size: "4096"
        image:  "bigtop/deploy:debian-8"
boot2docker:
        memory_size: "4096"
        number_cpus: "2"
repo: "https://ci.bigtop.apache.org/job/Bigtop-trunk/BUILD_ENVIRONMENTS=debian-8%2clabel=docker-slave/lastSuccessfulBuild/artifact/output/apt/"
distro: debian
components: [hadoop, qfs, bigtop-utils]
namenode_ui_port: "56110"
yarn_ui_port: "8211"
hbase_ui_port: "61110"
enable_local_repo: false
smoke_test_components: [qfs, hadoop]
jdk: "openjdk-7-jdk"
__EOT__

# destroy previous cluster
./gradlew docker-provisioner-destroy
# provision
./gradlew -Pconfig=vagrantconfig_Bigtop-Docker-provisioner-debian-8.yaml -Pnum_instances=3 -Prun_smoke_tests=true  docker-provisioner
# destroy provisinoed cluster
./gradlew docker-provisioner-destroy

with code from https://github.com/oflebbe/bigtop.git branch BIGTOP-2366

Right now it fails with a silly error message that bigtop-utils is not installed ?!?

You only need to explicitly specify helper packages like bigtop-utils. This is the list of components for puppet. Basically, Puppet will try to find the module with that name and install it, which will clearly fail. I suppose, removing bigtop-utils from your example should do the trick.

Hi guys,

Just wanted to follow up on this ticket. Any chance this can get reviewed and merged? Is there anything else you'd like me to focus on regarding this code?

Github user fsareshwala commented on a diff in the pull request:

https://github.com/apache/bigtop/pull/86#discussion_r61018978

— Diff: bigtop-tests/smoke-tests/qfs/build.gradle —
@@ -0,0 +1,40 @@
+/**
+ * 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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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.
+ */
+def tests_to_include()

+
+sourceSets {
+ test {
+ groovy {
+ srcDirs = [
+ "$

/bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/mapreduce"
+ ]
+ exclude

+ }
+ }
+}
+
+test.doFirst {
+ environment("HADOOP_COMMAND", "hadoop-qfs")
— End diff –

That's a good suggestion, @c0s. I've updated the code to check the environment variable `SMOKE_TESTS` before running the qfs smoke tests.

Github user c0s commented on a diff in the pull request:

https://github.com/apache/bigtop/pull/86#discussion_r61020136

— Diff: bigtop-tests/smoke-tests/qfs/build.gradle —
@@ -0,0 +1,40 @@
+/**
+ * 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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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.
+ */
+def tests_to_include()

+
+sourceSets {
+ test {
+ groovy {
+ srcDirs = [
+ "$

/bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/mapreduce"
+ ]
+ exclude

+ }
+ }
+}
+
+test.doFirst {
+ environment("HADOOP_COMMAND", "hadoop-qfs")
— End diff –

This isn't what I meant. Someone can set the SMOKE_TESTS variable to qfs as he is intent to run such tests. However, the cluster might not be configured with qfs, i.e. the packages aren't installed, etc. This will lead to a false negative. The reason I am being overly cautious is because in case of HDFS it is always there, so there's no need to specifically check for its presence. QFS being an optional alternative won't be implicitly required by other downstream components like YARN, hence it configuration might be omitted. See where I am going with this?

Github user fsareshwala commented on a diff in the pull request:

https://github.com/apache/bigtop/pull/86#discussion_r61158128

— Diff: bigtop-tests/smoke-tests/qfs/build.gradle —
@@ -0,0 +1,40 @@
+/**
+ * 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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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.
+ */
+def tests_to_include()

+
+sourceSets {
+ test {
+ groovy {
+ srcDirs = [
+ "$

/bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/mapreduce"
+ ]
+ exclude

+ }
+ }
+}
+
+test.doFirst {
+ environment("HADOOP_COMMAND", "hadoop-qfs")
— End diff –

I worry that checking for a binary existing might hide other problems with installing the binary itself. That's why I used the environment variable. One option would be to pass in the list of components to the `smoke-tests.sh` script from `docker-hadoop.sh` and I can search through that instead. What do you think about that? If that's not a good solution, can you give me a suggestion on how I might go about doing what you are asking for?

Github user c0s commented on a diff in the pull request:

https://github.com/apache/bigtop/pull/86#discussion_r61629098

— Diff: bigtop-tests/smoke-tests/qfs/build.gradle —
@@ -0,0 +1,40 @@
+/**
+ * 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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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.
+ */
+def tests_to_include()

+
+sourceSets {
+ test {
+ groovy {
+ srcDirs = [
+ "$

/bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/mapreduce"
+ ]
+ exclude

+ }
+ }
+}
+
+test.doFirst {
+ environment("HADOOP_COMMAND", "hadoop-qfs")
— End diff –

I guess you right. You know - let's scratch my initial suggestion and leave it as it was. After all other components don't do any checks like that and so far we haven't blown anything Thanks!

Github user fsareshwala commented on a diff in the pull request:

https://github.com/apache/bigtop/pull/86#discussion_r61629748

— Diff: bigtop-tests/smoke-tests/qfs/build.gradle —
@@ -0,0 +1,40 @@
+/**
+ * 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
+ * <p/>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p/>
+ * 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.
+ */
+def tests_to_include()

+
+sourceSets {
+ test {
+ groovy {
+ srcDirs = [
+ "$

/bigtop-tests/test-artifacts/hadoop/src/main/groovy/org/apache/bigtop/itest/hadoop/mapreduce"
+ ]
+ exclude

+ }
+ }
+}
+
+test.doFirst {
+ environment("HADOOP_COMMAND", "hadoop-qfs")
— End diff –

Sure thing @c0s. I still think it's valuable to check for the `SMOKE_TESTS` environment variable inside the qfs `build.gradle`. I've left that in. I think if you are ready for the merge, this is ready to go. Let me know if there's anything else you want me to change.

Pushed to the master, thanks Faraaz Sareshwala

Please reassign this one to me. I work on QFS and am looking at added the tests.

Sorry. Commented on the wrong ticket.