[BEAM-3214] Add integration test for HBaseIO. by szewi · Pull Request #5499 · apache/beam

szewi · 2018-05-28T21:14:43Z

This PR contains:

kubernetes infrastructure for running integration tests of HBaseIO
support for running integration test of HBaseIO with Gradle
java code of integration test
by default test is being run on 100k records, but hashes are also calculated for 1k, 600k and 5M of records and can be set with pipeline option --numberOfRecords

Follow this checklist to help us incorporate your contribution quickly and easily:

Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

lgajowy · 2018-05-29T08:53:15Z

R: @iemejia

iemejia · 2018-05-29T08:57:22Z

CC: @aromanenko-dev

iemejia

Ultra minor nitpicks, execellent work @szewi. I will wait for your fixes and in the meantime try to reproduce the k8s scripts. Did you have any chance creating a multi node cluster or this is for another PR ?

iemejia · 2018-05-29T09:08:06Z

.test-infra/kubernetes/hbase/SmallITCluster/hbase-single-node-cluster.yml

+    spec:
+      containers:
+        - name: hbase
+          image: harisekhon/hbase:latest


We should probably pin this to version 1.2 since this is the official stable one and the one the connector uses.

The latest points to 1.3 version currently, but to be strict with the version specified in build.gradle I will pin this to 1.2 as there is docker tag with this version available.

iemejia · 2018-05-29T09:42:10Z

sdks/java/io/common/src/test/java/org/apache/beam/sdk/io/common/IOTestPipelineOptions.java

+  /* HBase */
+  @Description("HBase host")
+  @Default.String("HBase-host")
+  String getHbaseServerName();


We decided to not expand this Options object anymore. Please create a PipelineOptions specific for HBase see HCatalogIO for ref.

Thanks, I wasn't aware of that.

iemejia · 2018-05-29T09:42:37Z

sdks/java/io/hbase/src/test/java/org/apache/beam/sdk/io/hbase/HBaseIOIT.java

+  /**
+   * Produces test rows.
+   */
+  public static class ConstructMutations extends DoFn<TestRow, Mutation> {


iemejia · 2018-05-29T09:42:46Z

sdks/java/io/hbase/src/test/java/org/apache/beam/sdk/io/hbase/HBaseIOIT.java

+  /**
+   * Read rows from Table.
+   */
+  public static class SelectNameFn extends DoFn<Result, String> {


iemejia · 2018-05-29T09:59:31Z

sdks/java/io/hbase/src/test/java/org/apache/beam/sdk/io/hbase/HBaseIOIT.java

+ */
+@RunWith(JUnit4.class)
+public class HBaseIOIT {
+  private static final Logger LOG = LoggerFactory.getLogger(HBaseIOIT.class);


This is never used so maybe remove.

iemejia · 2018-05-29T10:02:21Z

.test-infra/kubernetes/hbase/SmallITCluster/setup-all.sh

+set -e
+
+kubectl create -f hbase-single-node-cluster.yml
+


I would remove this space and the one in line 28 (ultra minor nitpick)

iemejia · 2018-05-29T10:02:44Z

.test-infra/kubernetes/hbase/SmallITCluster/setup.sh

+#    See the License for the specific language governing permissions and
+#    limitations under the License.
+#
+# Start HBase cluster.


remove this one and the next line to be consistent with the other setup.sh files

iemejia · 2018-05-29T10:03:04Z

.test-infra/kubernetes/hbase/SmallITCluster/teardown-all.sh

+external_ip="$(kubectl get svc hbase-external -o jsonpath='{.status.loadBalancer.ingress[0].ip}')"
+
+hbase_master_pod_name="$(kubectl get pods --selector=name=hbase -o jsonpath='{.items[*].metadata.name}')"
+


same comment for spaces as above

iemejia · 2018-05-29T10:16:22Z

.test-infra/kubernetes/hbase/SmallITCluster/hbase-single-node-cluster-for-local-dev.yml

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


Please remove the '# char so this does not get mingled with the ASF license.

iemejia · 2018-05-29T10:16:34Z

.test-infra/kubernetes/hbase/SmallITCluster/hbase-single-node-cluster.yml

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


Same as above

iemejia · 2018-05-29T10:16:53Z

.test-infra/kubernetes/hbase/SmallITCluster/setup-all.sh

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


same as above, remove # char

iemejia · 2018-05-29T10:17:08Z

.test-infra/kubernetes/hbase/SmallITCluster/teardown-all.sh

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


remove # char also

iemejia · 2018-05-29T10:17:23Z

.test-infra/kubernetes/hbase/SmallITCluster/teardown.sh

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


remove # char

iemejia · 2018-05-29T10:17:37Z

sdks/java/io/hbase/src/test/java/org/apache/beam/sdk/io/hbase/HBaseIOIT.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+


remove space

iemejia · 2018-05-29T10:17:49Z

sdks/java/io/hbase/src/test/java/org/apache/beam/sdk/io/hbase/HBaseIOIT.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+


remove space

iemejia · 2018-05-29T10:19:20Z

sdks/java/io/hbase/src/test/java/org/apache/beam/sdk/io/hbase/HBaseIOIT.java

+ *
+ */
+@RunWith(JUnit4.class)
+public class HBaseIOIT {


Do you use IntelliJ if so can you please format this file with the google-java-plugin (this is not mandatory but it is what most people here use and probably will be automatized in the future).

iemejia · 2018-05-29T13:07:18Z

.test-infra/kubernetes/hbase/SmallITCluster/setup-all.sh

+# should be added to /etc/hosts file in order to work with HBase cluster.
+#
+
+#!/bin/sh


shebangs must be the first line in the file so probably just move it to the top in this in all the shell files.

#!/bin/sh # ...

Extra points if you open a subsequent PR to fix this in all the other shell scripts (the non-hbase ones) (+ eventual fixes for shellcheck issues).

Ok this makes sense. In this PR I just wanted to be consistent with current script pattern, but this could be easily fixed.

iemejia · 2018-05-29T13:09:35Z

.test-infra/kubernetes/hbase/SmallITCluster/setup-all.sh

+hbase_master_namespace="$(kubectl get pods --selector=name=hbase -o jsonpath='{.items[*].metadata.namespace}')"
+
+echo "For local tests please add the following entry to /etc/hosts file"
+echo ${external_ip}$'\t'${hbase_master_pod_name}".hbase.${hbase_master_namespace}.svc.cluster.local"


SC2086: Double quote to prevent globbing and word splitting.
^-- SC2039: In POSIX sh, $'..' is undefined.
^-- SC2086: Double quote to prevent globbing and word splitting.

Pro tip: always pass a run of shellcheck in the shell scripts for specific issues like this.
https://www.shellcheck.net/

iemejia · 2018-05-29T13:12:41Z

.test-infra/kubernetes/hbase/SmallITCluster/setup.sh

+#!/bin/sh
+set -e
+
+kubectl create -f hbase-single-node.yml


when running the script it complaines on missing file, maybe it is hbase-single-node-cluster.yml ?

exactly I was used different naming convention at the beginning of working at this PR

iemejia · 2018-05-29T13:14:45Z

.test-infra/kubernetes/hbase/SmallITCluster/teardown.sh

+#!/bin/sh
+set -e
+
+kubectl delete -f hbase-single-node.yml


when running the script it complaines on missing file, maybe it is hbase-single-node-cluster.yml ?

szewi · 2018-06-04T09:48:59Z

@iemejia this is ready for another round.
BTW to reproduce kubernetes infra you need to upgrade your cluster and kubectl client tool to at least 1.9.3, but latest 1.9.X is preferred, otherwise you may expect some errors with creation/deletion of the pods using StatefulSets.

iemejia

LGTM, Thanks @szewi. Waiting for jenkins to merge.

szewi · 2018-06-04T15:16:47Z

Run Java PostCommit

szewi · 2018-06-04T15:17:48Z

Run Java PreCommit

szewi · 2018-06-04T15:18:48Z

@iemejia sorry I run post commit tests by mistake instead of precommit. Now waiting for 2 checks to be completed.

iemejia

LGTM.
Thanks @szewi nice to have this one!

iemejia self-requested a review May 29, 2018 08:54

iemejia requested changes May 29, 2018

View reviewed changes

iemejia reviewed May 29, 2018

View reviewed changes

szewi force-pushed the hbase-io branch from 94e5735 to 098ac31 Compare June 4, 2018 14:18

[BEAM-3214] Add integration test for HBaseIO.

0e0da28

szewi force-pushed the hbase-io branch from 098ac31 to 0e0da28 Compare June 4, 2018 14:32

iemejia approved these changes Jun 4, 2018

View reviewed changes

iemejia approved these changes Jun 5, 2018

View reviewed changes

iemejia merged commit f89e19b into apache:master Jun 6, 2018

		external_ip="$(kubectl get svc hbase-external -o jsonpath='{.status.loadBalancer.ingress[0].ip}')"

		hbase_master_pod_name="$(kubectl get pods --selector=name=hbase -o jsonpath='{.items[*].metadata.name}')"

		import org.slf4j.Logger;
		import org.slf4j.LoggerFactory;

Conversation

szewi commented May 28, 2018

Uh oh!

lgajowy commented May 29, 2018

Uh oh!

iemejia commented May 29, 2018

Uh oh!

iemejia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iemejia May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iemejia May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iemejia May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szewi commented Jun 4, 2018

Uh oh!

iemejia left a comment

Choose a reason for hiding this comment

Uh oh!

szewi commented Jun 4, 2018

Uh oh!

szewi commented Jun 4, 2018

Uh oh!

szewi commented Jun 4, 2018

Uh oh!

iemejia left a comment

Choose a reason for hiding this comment

Uh oh!

iemejia May 29, 2018 •

edited

Loading

iemejia May 29, 2018 •

edited

Loading

iemejia May 29, 2018 •

edited

Loading