Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.1.0
    • Fix Version/s: 1.2.0
    • Component/s: build, flink, general
    • Labels:
      None
    1. 0001-BIGTOP-2345-Create-Flink-packaging.patch
      32 kB
      Konstantin Boudnik
    2. flink.patch
      9 kB
      jay vyas

      Activity

      Hide
      jayunit100 jay vyas added a comment -

      for discussion only, first hack attempt at a patch by bhupendra

      Show
      jayunit100 jay vyas added a comment - for discussion only, first hack attempt at a patch by bhupendra
      Hide
      jayunit100 jay vyas added a comment - - edited

      haven't done much packaging but looking at this patch Konstantin Boudnik or Olaf Flebbe ... any obvious mistakes come to mind?

      This is being done by a group of graduate students in a class I'm guest lecturing, so go easy on em

      Show
      jayunit100 jay vyas added a comment - - edited haven't done much packaging but looking at this patch Konstantin Boudnik or Olaf Flebbe ... any obvious mistakes come to mind? This is being done by a group of graduate students in a class I'm guest lecturing, so go easy on em
      Hide
      oflebbe Olaf Flebbe added a comment -

      Hi,

      This looks good to me, congrats! Though I could not test it right now.

      Only 3 nitty picky things, and an obvious larger one:

      • The patch should be best relative to master branch not 1.1.0.
      • The export {{+export SPARK_HOME=\$ {SPARK_HOME:-/usr/lib/spark}

        }} does not make sense here, since the comand above is an exec

      • I like the subsequent tries of patches numbered (for instance BIGTOP-2345.1.patch ... BIGTOP-2345.2.patch). Its easier to check what's changed if I can download them without sorting them by time.

      And the obvious one (Don't know whether that was in your focus.) We need the debian packaging as well. Hope your install _fink.sh is flexible enough for that kind of install layout.

      And the big question (I don't know flink at all), if I look at the Apache Flink top page:
      I see server side components. (YARN for instance). Doesn't flink have services to start or at least things to copy to HDFS?

      Show
      oflebbe Olaf Flebbe added a comment - Hi, This looks good to me, congrats! Though I could not test it right now. Only 3 nitty picky things, and an obvious larger one: The patch should be best relative to master branch not 1.1.0. The export {{+export SPARK_HOME=\$ {SPARK_HOME:-/usr/lib/spark} }} does not make sense here, since the comand above is an exec I like the subsequent tries of patches numbered (for instance BIGTOP-2345 .1.patch ... BIGTOP-2345 .2.patch). Its easier to check what's changed if I can download them without sorting them by time. And the obvious one (Don't know whether that was in your focus.) We need the debian packaging as well. Hope your install _fink.sh is flexible enough for that kind of install layout. And the big question (I don't know flink at all), if I look at the Apache Flink top page: I see server side components. (YARN for instance). Doesn't flink have services to start or at least things to copy to HDFS?
      Hide
      rmetzger Robert Metzger added a comment -

      > Doesn't flink have services to start or at least things to copy to HDFS?

      Flink does not require HDFS to run, if that's answering your question.

      Show
      rmetzger Robert Metzger added a comment - > Doesn't flink have services to start or at least things to copy to HDFS? Flink does not require HDFS to run, if that's answering your question.
      Hide
      jayunit100 jay vyas added a comment -

      Additionally, We aren't packaging flink as a Yarn app but as standalone, following the model for spark

      Show
      jayunit100 jay vyas added a comment - Additionally, We aren't packaging flink as a Yarn app but as standalone, following the model for spark
      Hide
      rmetzger Robert Metzger added a comment -

      +1 that makes sense for bigtop in my opinion!
      In that case, Flink is really independent of the file systems. You can easily use Flink with Google cloud storage ("gs:///") or Amazon S3.

      Show
      rmetzger Robert Metzger added a comment - +1 that makes sense for bigtop in my opinion! In that case, Flink is really independent of the file systems. You can easily use Flink with Google cloud storage ("gs:///") or Amazon S3.
      Hide
      Bhupendra Singh Bhupendra Singh added a comment -

      Hi,
      thanks for the update. surely looking into the issues. have resolved issues reported by Olaf Flebbe. Will update the patch as soon as possible. Working fine now for majority of the build process. Still minor script changes remaining.

      Show
      Bhupendra Singh Bhupendra Singh added a comment - Hi, thanks for the update. surely looking into the issues. have resolved issues reported by Olaf Flebbe . Will update the patch as soon as possible. Working fine now for majority of the build process. Still minor script changes remaining.
      Hide
      mxm Maximilian Michels added a comment -

      Hi Bhupendra! What is the current state of the patch? Did you make any further changes? I would like to resolve any pending issues so we can open a pull request.

      Show
      mxm Maximilian Michels added a comment - Hi Bhupendra ! What is the current state of the patch? Did you make any further changes? I would like to resolve any pending issues so we can open a pull request.
      Hide
      mxm Maximilian Michels added a comment -

      Here is the pending pull request: https://github.com/apache/bigtop/pull/93/

      Show
      mxm Maximilian Michels added a comment - Here is the pending pull request: https://github.com/apache/bigtop/pull/93/
      Hide
      oflebbe Olaf Flebbe added a comment -
      Show
      oflebbe Olaf Flebbe added a comment - See for for instance {{ https://ci.bigtop.apache.org/job/Bigtop-pullrequest93/BUILD_ENVIRONMENTS=centos-7,label=docker-slave/4/console }} The patch does not apply cleanly
      Hide
      rmetzger Robert Metzger added a comment -

      Thank you for looking into this Olaf.
      I've started reworking the pull request today to make it work. I hope I'm finished with the rework tomorrow.

      Show
      rmetzger Robert Metzger added a comment - Thank you for looking into this Olaf. I've started reworking the pull request today to make it work. I hope I'm finished with the rework tomorrow.
      Hide
      oflebbe Olaf Flebbe added a comment -

      Great. The compile job should run every two hours. But I am not sure you can rebase the pull request #93

      Show
      oflebbe Olaf Flebbe added a comment - Great. The compile job should run every two hours. But I am not sure you can rebase the pull request #93
      Hide
      oflebbe Olaf Flebbe added a comment -

      And BTW I can send you the command lines for you to test building within the bigtop environment. All you need is a running docker .

      Show
      oflebbe Olaf Flebbe added a comment - And BTW I can send you the command lines for you to test building within the bigtop environment. All you need is a running docker .
      Hide
      githubbot ASF GitHub Bot added a comment -

      GitHub user rmetzger opened a pull request:

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

      BIGTOP-2345 Add deb and rpm packaging for Apache Flink

      This PR is based on the work done in https://github.com/apache/bigtop/pull/93.
      I've picked only the parts regarding the packaging (ignoring the puppet deployment).

      The changes have been tested on a Debian8 virtual machine and Fedora 23.
      Packaging, installation, the init scripts, user creation, file removal, logging seem to work now.

      I'm looking forward to any comments regarding the proposed changes.

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

      $ git pull https://github.com/rmetzger/bigtop bigtop2345-pr

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

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


      commit f076772ba20ad5a9590c479987e95292e930a752
      Author: bhupendrasingh <bhupendrasingh.2@gmail.com>
      Date: 2016-02-20T21:47:27Z

      BIGTOP-2345 Initial prototyping for Flink packaging

      commit 1e67d973bf36644e3e13fb5a287b36b72ab2d120
      Author: Robert Metzger <rmetzger@apache.org>
      Date: 2016-04-20T15:59:54Z

      BIGTOP-2345 Refined Flink packaging


      Show
      githubbot ASF GitHub Bot added a comment - GitHub user rmetzger opened a pull request: https://github.com/apache/bigtop/pull/101 BIGTOP-2345 Add deb and rpm packaging for Apache Flink This PR is based on the work done in https://github.com/apache/bigtop/pull/93 . I've picked only the parts regarding the packaging (ignoring the puppet deployment). The changes have been tested on a Debian8 virtual machine and Fedora 23. Packaging, installation, the init scripts, user creation, file removal, logging seem to work now. I'm looking forward to any comments regarding the proposed changes. You can merge this pull request into a Git repository by running: $ git pull https://github.com/rmetzger/bigtop bigtop2345-pr Alternatively you can review and apply these changes as the patch at: https://github.com/apache/bigtop/pull/101.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 #101 commit f076772ba20ad5a9590c479987e95292e930a752 Author: bhupendrasingh <bhupendrasingh.2@gmail.com> Date: 2016-02-20T21:47:27Z BIGTOP-2345 Initial prototyping for Flink packaging commit 1e67d973bf36644e3e13fb5a287b36b72ab2d120 Author: Robert Metzger <rmetzger@apache.org> Date: 2016-04-20T15:59:54Z BIGTOP-2345 Refined Flink packaging
      Hide
      rmetzger Robert Metzger added a comment -

      I can not change other people's pull requests.
      I've opened a new one for this issue.

      If its not much overhead for you, it would be great if you could paste me the command lines for testing my stuff.
      (I have docker, but I never got around trying it really out )
      I've manually tested my changes, but I guess some automated "official" tests can not hurt.

      Show
      rmetzger Robert Metzger added a comment - I can not change other people's pull requests. I've opened a new one for this issue. If its not much overhead for you, it would be great if you could paste me the command lines for testing my stuff. (I have docker, but I never got around trying it really out ) I've manually tested my changes, but I guess some automated "official" tests can not hurt.
      Hide
      githubbot ASF GitHub Bot added a comment -

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

      https://github.com/apache/bigtop/pull/101#discussion_r60749429

      — Diff: bigtop-packages/src/rpm/flink/SPECS/flink.spec —
      @@ -0,0 +1,143 @@
      +# 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
      +#
      +# http://www.apache.org/licenses/LICENSE-2.0
      +#
      +# 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.
      +
      +%define flink_name flink
      +%define lib_flink /usr/lib/%

      {flink_name}
      +%define bin_flink /usr/bin
      +%define etc_flink /etc/%{flink_name}

      +%define config_flink %

      {etc_flink}

      /conf
      +%define man_dir %{_mandir}
      +%define flink_services jobmanager taskmanager
      +%define var_run_flink /var/run/%

      {flink_name}
      +%define var_log_flink /var/log/%{flink_name}

      +
      +
      +%if %

      {!?suse_version:1}

      0
      +%define doc_flink %{_docdir}/%

      {flink_name}-%{flink_version}
      +%define alternatives_cmd alternatives
      +%define build_flink %{_builddir}/%{flink_name}

      -%

      {flink_version}/flink-dist/target/%{flink_name}-%{flink_version}

      -bin/%

      {flink_name}-%{flink_version}/
      +
      +%else
      +%define doc_flink %{_docdir}/%{flink_name}

      -%

      {flink_version}
      +%define alternatives_cmd update-alternatives
      +%endif
      +
      +Name: %{flink_name}
      +Version: %{flink_version}

      +Release: %

      {flink_release}

      +Summary: Apache Flink is an open source platform for distributed stream and batch data processing.
      +License: ASL 2.0
      +URL: http://flink.apache.org/
      +Group: Development/Libraries
      +Buildroot: %{_topdir}/INSTALL/%

      {name}-%{version}
      +BuildArch: noarch
      +Source0: flink-%{flink_base_version}.tar.gz
      +Source1: do-component-build
      +Source2: install_flink.sh
      +Source3: init.d.tmpl
      +Source4: jobmanager.svc
      +Source5: taskmanager.svc
      +Source6: bigtop.bom
      +Requires: bigtop-utils >= 0.7
      +Requires(preun): /sbin/service
      +
      +%description
      +Apache Flink is an open source platform for distributed stream and batch data processing.
      +Flink’s core is a streaming dataflow engine that provides data distribution, communication,
      +and fault tolerance for distributed computations over data streams.
      +
      +Flink includes several APIs for creating applications that use the Flink engine:
      + * DataStream API for unbounded streams embedded in Java and Scala, and
      + * DataSet API for static data embedded in Java, Scala, and Python,
      + * Table API with a SQL-like expression language embedded in Java and Scala.
      +
      +Flink also bundles libraries for domain-specific use cases:
      + * Machine Learning library, and
      + * Gelly, a graph processing API and library.
      +
      +Some of the key features of Apache Flink includes.
      + * Complete Event Processing (CEP)
      + * Fault-tolerance via Lightweight Distributed Snapshots
      + * Hadoop-native YARN & HDFS implementation
      +
      +# Additions for master-worker configuration #
      +
      +%global initd_dir %{_sysconfdir}/init.d
      +
      +%if %{?suse_version:1}0
      +# Required for init scripts
      +Requires: insserv
      +%global initd_dir %{_sysconfdir}/rc.d
      +
      +%else
      +# Required for init scripts
      +Requires: /lib/lsb/init-functions
      +%global initd_dir %{_sysconfdir}/rc.d/init.d
      +%endif
      +
      +##############################################
      +
      +%prep
      +%setup -n %{name}

      -%

      {flink_base_version}


      +
      +%build
      +bash $RPM_SOURCE_DIR/do-component-build
      +
      +
      +
      +# Init.d scripts
      +%__install -d -m 0755 $RPM_BUILD_ROOT/%

      {initd_dir}/
      +
      +%install
      +%__rm -rf $RPM_BUILD_ROOT
      +
      +sh -x %{SOURCE2} --prefix=$RPM_BUILD_ROOT --source-dir=$RPM_SOURCE_DIR --build-dir=`pwd`/build-target
      +
      +
      +
      +for service in %{flink_services}
      +do
      + # Install init script
      + init_file=$RPM_BUILD_ROOT/%{initd_dir}

      /%

      {flink_name}-${service}
      + bash %{SOURCE3} $RPM_SOURCE_DIR/%{flink_name}

      -$

      {service}.svc rpm $init_file
      +done
      +
      +
      +%preun
      +for service in %{flink_services}; do
      + /sbin/service %{flink_name}-${service}

      status > /dev/null 2>&1
      + if [ $? -eq 0 ]; then
      + /sbin/service %

      {flink_name}-${service} stop > /dev/null 2>&1
      + fi
      +done
      +
      +
      +%post
      +%{alternatives_cmd} --install %{config_flink} %{flink_name}

      -conf %

      {config_flink}.dist 30
      +
      +###### FILES ###########
      +
      +%files
      +%defattr(-,root,root,755)
      +%config(noreplace) %{config_flink}

      .dist
      +
      +%dir %{_sysconfdir}/%

      {flink_name}
      +%config(noreplace) %{initd_dir}/%{flink_name}

      -master
      +%config(noreplace) %

      {initd_dir}/%{flink_name}-worker
      — End diff –

      I think those need to be
      ````
      %config(noreplace) %{initd_dir}

      /%

      {flink_name}-jobmanager
      %config(noreplace) %{initd_dir}/%{flink_name}

      -taskmanager
      ```

      Show
      githubbot ASF GitHub Bot added a comment - Github user mxm commented on a diff in the pull request: https://github.com/apache/bigtop/pull/101#discussion_r60749429 — Diff: bigtop-packages/src/rpm/flink/SPECS/flink.spec — @@ -0,0 +1,143 @@ +# 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 +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# 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. + +%define flink_name flink +%define lib_flink /usr/lib/% {flink_name} +%define bin_flink /usr/bin +%define etc_flink /etc/%{flink_name} +%define config_flink % {etc_flink} /conf +%define man_dir %{_mandir} +%define flink_services jobmanager taskmanager +%define var_run_flink /var/run/% {flink_name} +%define var_log_flink /var/log/%{flink_name} + + +%if % {!?suse_version:1} 0 +%define doc_flink %{_docdir}/% {flink_name}-%{flink_version} +%define alternatives_cmd alternatives +%define build_flink %{_builddir}/%{flink_name} -% {flink_version}/flink-dist/target/%{flink_name}-%{flink_version} -bin/% {flink_name}-%{flink_version}/ + +%else +%define doc_flink %{_docdir}/%{flink_name} -% {flink_version} +%define alternatives_cmd update-alternatives +%endif + +Name: %{flink_name} +Version: %{flink_version} +Release: % {flink_release} +Summary: Apache Flink is an open source platform for distributed stream and batch data processing. +License: ASL 2.0 +URL: http://flink.apache.org/ +Group: Development/Libraries +Buildroot: %{_topdir}/INSTALL/% {name}-%{version} +BuildArch: noarch +Source0: flink-%{flink_base_version}.tar.gz +Source1: do-component-build +Source2: install_flink.sh +Source3: init.d.tmpl +Source4: jobmanager.svc +Source5: taskmanager.svc +Source6: bigtop.bom +Requires: bigtop-utils >= 0.7 +Requires(preun): /sbin/service + +%description +Apache Flink is an open source platform for distributed stream and batch data processing. +Flink’s core is a streaming dataflow engine that provides data distribution, communication, +and fault tolerance for distributed computations over data streams. + +Flink includes several APIs for creating applications that use the Flink engine: + * DataStream API for unbounded streams embedded in Java and Scala, and + * DataSet API for static data embedded in Java, Scala, and Python, + * Table API with a SQL-like expression language embedded in Java and Scala. + +Flink also bundles libraries for domain-specific use cases: + * Machine Learning library, and + * Gelly, a graph processing API and library. + +Some of the key features of Apache Flink includes. + * Complete Event Processing (CEP) + * Fault-tolerance via Lightweight Distributed Snapshots + * Hadoop-native YARN & HDFS implementation + +# Additions for master-worker configuration # + +%global initd_dir %{_sysconfdir}/init.d + +%if %{?suse_version:1}0 +# Required for init scripts +Requires: insserv +%global initd_dir %{_sysconfdir}/rc.d + +%else +# Required for init scripts +Requires: /lib/lsb/init-functions +%global initd_dir %{_sysconfdir}/rc.d/init.d +%endif + +############################################## + +%prep +%setup -n %{name} -% {flink_base_version} + +%build +bash $RPM_SOURCE_DIR/do-component-build + + + +# Init.d scripts +%__install -d -m 0755 $RPM_BUILD_ROOT/% {initd_dir}/ + +%install +%__rm -rf $RPM_BUILD_ROOT + +sh -x %{SOURCE2} --prefix=$RPM_BUILD_ROOT --source-dir=$RPM_SOURCE_DIR --build-dir=`pwd`/build-target + + + +for service in %{flink_services} +do + # Install init script + init_file=$RPM_BUILD_ROOT/%{initd_dir} /% {flink_name}-${service} + bash %{SOURCE3} $RPM_SOURCE_DIR/%{flink_name} -$ {service}.svc rpm $init_file +done + + +%preun +for service in %{flink_services}; do + /sbin/service %{flink_name}-${service} status > /dev/null 2>&1 + if [ $? -eq 0 ]; then + /sbin/service % {flink_name}-${service} stop > /dev/null 2>&1 + fi +done + + +%post +%{alternatives_cmd} --install %{config_flink} %{flink_name} -conf % {config_flink}.dist 30 + +###### FILES ########### + +%files +%defattr(-,root,root,755) +%config(noreplace) %{config_flink} .dist + +%dir %{_sysconfdir}/% {flink_name} +%config(noreplace) %{initd_dir}/%{flink_name} -master +%config(noreplace) % {initd_dir}/%{flink_name}-worker — End diff – I think those need to be ```` %config(noreplace) %{initd_dir} /% {flink_name}-jobmanager %config(noreplace) %{initd_dir}/%{flink_name} -taskmanager ```
      Hide
      githubbot ASF GitHub Bot added a comment -

      Github user mxm commented on the pull request:

      https://github.com/apache/bigtop/pull/101#issuecomment-213452902

      I'm getting the following error message when building using `gradle flink-rpm`:

      >RPM build errors:
      File not found: /home/max/Desktop/bigtop/build/flink/rpm/BUILDROOT/flink-1.0.0-1.fc23.x86_64/etc/rc.d/init.d/flink-master
      File not found: /home/max/Desktop/bigtop/build/flink/rpm/BUILDROOT/flink-1.0.0-1.fc23.x86_64/etc/rc.d/init.d/flink-worker
      File not found: /home/max/Desktop/bigtop/build/flink/rpm/BUILDROOT/flink-1.0.0-1.fc23.x86_64/usr/share/doc/flink-1.0.0

      Show
      githubbot ASF GitHub Bot added a comment - Github user mxm commented on the pull request: https://github.com/apache/bigtop/pull/101#issuecomment-213452902 I'm getting the following error message when building using `gradle flink-rpm`: >RPM build errors: File not found: /home/max/Desktop/bigtop/build/flink/rpm/BUILDROOT/flink-1.0.0-1.fc23.x86_64/etc/rc.d/init.d/flink-master File not found: /home/max/Desktop/bigtop/build/flink/rpm/BUILDROOT/flink-1.0.0-1.fc23.x86_64/etc/rc.d/init.d/flink-worker File not found: /home/max/Desktop/bigtop/build/flink/rpm/BUILDROOT/flink-1.0.0-1.fc23.x86_64/usr/share/doc/flink-1.0.0
      Hide
      oflebbe Olaf Flebbe added a comment -
      Show
      oflebbe Olaf Flebbe added a comment - You can follow https://ci.bigtop.apache.org/job/Bigtop-pullrequest101/ now.
      Hide
      rmetzger Robert Metzger added a comment -

      Thank you. I'm not a Jenkins expert, but I don't think that this error is caused by my pull request:
      https://ci.bigtop.apache.org/job/Bigtop-pullrequest101/BUILD_ENVIRONMENTS=fedora-20,label=docker-slave/lastBuild/consoleFull
      Can you look into it?

      Show
      rmetzger Robert Metzger added a comment - Thank you. I'm not a Jenkins expert, but I don't think that this error is caused by my pull request: https://ci.bigtop.apache.org/job/Bigtop-pullrequest101/BUILD_ENVIRONMENTS=fedora-20,label=docker-slave/lastBuild/consoleFull Can you look into it?
      Hide
      oflebbe Olaf Flebbe added a comment -

      Aehm, sorry. That's not an jenkins issue, that's me. Fixed and running again.

      Show
      oflebbe Olaf Flebbe added a comment - Aehm, sorry. That's not an jenkins issue, that's me. Fixed and running again.
      Hide
      githubbot ASF GitHub Bot added a comment -

      Github user oflebbe commented on the pull request:

      https://github.com/apache/bigtop/pull/101#issuecomment-214708194

      Hi the build looks very promising! You should not package /var/run, since this is only a temporary filesystem. You should create it with your init script (This will fix one of the lintian errors for debian/ubuntu).

      Show
      githubbot ASF GitHub Bot added a comment - Github user oflebbe commented on the pull request: https://github.com/apache/bigtop/pull/101#issuecomment-214708194 Hi the build looks very promising! You should not package /var/run, since this is only a temporary filesystem. You should create it with your init script (This will fix one of the lintian errors for debian/ubuntu).
      Hide
      githubbot ASF GitHub Bot added a comment -

      Github user rmetzger commented on the pull request:

      https://github.com/apache/bigtop/pull/101#issuecomment-214720306

      Thank you for the feedback!
      I'm trying to address them soon.

      Show
      githubbot ASF GitHub Bot added a comment - Github user rmetzger commented on the pull request: https://github.com/apache/bigtop/pull/101#issuecomment-214720306 Thank you for the feedback! I'm trying to address them soon.
      Hide
      githubbot ASF GitHub Bot added a comment -

      Github user rmetzger commented on the pull request:

      https://github.com/apache/bigtop/pull/101#issuecomment-214785025

      Thank you for the review @mxm!

      Regarding the lsb dependency: I saw that the Hadoop spec for example has the following entry:
      ```
      Requires: sh-utils, /lib/lsb/init-functions
      ```
      I guess we can add that as well.

      > I wonder, should we rename the service to flink-master and flink-worker? Or flink-jobmanager and flink-taskmanager? Simply TaskManager and JobManager could confuse people.

      I'm definitively against `flink-master`, `flink-worker`, because that's not an "official" terminology (at least in the Flink code we call them job- and taskmanager).
      I also thought about `flink-jobmanager` and `flink-taskmanager`. Any other thoughts on this?

      > Another thing, the logs are named "flink-flink-jobmanager-0-localhost.localdomain.log". Probably could remove the flink-flink prefix.

      The naming schema for logs is `flink-<username><component><instance>-<host>.<log|out>`. By starting the flink services with the `flink` user, we end up with that ugliness.
      I'll look into it and try to see if I can change the behavior easily (the problem is that I need to create patches if I want to change anything on the flink source, and I want to avoid patching too much for bigtop).

      @oflebbe: I'll check the guide again and adopt the patch integration accordingly.

      Show
      githubbot ASF GitHub Bot added a comment - Github user rmetzger commented on the pull request: https://github.com/apache/bigtop/pull/101#issuecomment-214785025 Thank you for the review @mxm! Regarding the lsb dependency: I saw that the Hadoop spec for example has the following entry: ``` Requires: sh-utils, /lib/lsb/init-functions ``` I guess we can add that as well. > I wonder, should we rename the service to flink-master and flink-worker? Or flink-jobmanager and flink-taskmanager? Simply TaskManager and JobManager could confuse people. I'm definitively against `flink-master`, `flink-worker`, because that's not an "official" terminology (at least in the Flink code we call them job- and taskmanager). I also thought about `flink-jobmanager` and `flink-taskmanager`. Any other thoughts on this? > Another thing, the logs are named "flink-flink-jobmanager-0-localhost.localdomain.log". Probably could remove the flink-flink prefix. The naming schema for logs is `flink-<username> <component> <instance>-<host>.<log|out>`. By starting the flink services with the `flink` user, we end up with that ugliness. I'll look into it and try to see if I can change the behavior easily (the problem is that I need to create patches if I want to change anything on the flink source, and I want to avoid patching too much for bigtop). @oflebbe: I'll check the guide again and adopt the patch integration accordingly.
      Hide
      githubbot ASF GitHub Bot added a comment -

      Github user rmetzger commented on the pull request:

      https://github.com/apache/bigtop/pull/101#issuecomment-214794072

      @oflebbe quick question:
      The spec file contains already the following section:

      ```spec
      %if %{?suse_version:1}0

      1. Required for init scripts
        Requires: insserv
        %global initd_dir %{_sysconfdir}/rc.d

      %else

      1. Required for init scripts
        Requires: /lib/lsb/init-functions
        %global initd_dir %{_sysconfdir}/rc.d/init.d
        %endif
        ```

      I assume the if condition is true for suse, so the else branch applied for Max' build, requiring the init-functions?

      Show
      githubbot ASF GitHub Bot added a comment - Github user rmetzger commented on the pull request: https://github.com/apache/bigtop/pull/101#issuecomment-214794072 @oflebbe quick question: The spec file contains already the following section: ```spec %if %{?suse_version:1}0 Required for init scripts Requires: insserv %global initd_dir %{_sysconfdir}/rc.d %else Required for init scripts Requires: /lib/lsb/init-functions %global initd_dir %{_sysconfdir}/rc.d/init.d %endif ``` I assume the if condition is true for suse, so the else branch applied for Max' build, requiring the init-functions?
      Hide
      githubbot ASF GitHub Bot added a comment -

      Github user rmetzger commented on the pull request:

      https://github.com/apache/bigtop/pull/101#issuecomment-214798709

      Another follow up Q:

      > You should not package /var/run, since this is only a temporary filesystem. You should create it with your init script (This will fix one of the lintian errors for debian/ubuntu).

      Okay, makes sense. I was looking a bit into other scripts and I saw that others are setting the `$WORKING_DIR` variable.
      I wonder why `/bigtop-packages/src/templates/init.d.tmpl` is not taking care of the creation and deletion of the `$WORKING_DIR` for that?

      Show
      githubbot ASF GitHub Bot added a comment - Github user rmetzger commented on the pull request: https://github.com/apache/bigtop/pull/101#issuecomment-214798709 Another follow up Q: > You should not package /var/run, since this is only a temporary filesystem. You should create it with your init script (This will fix one of the lintian errors for debian/ubuntu). Okay, makes sense. I was looking a bit into other scripts and I saw that others are setting the `$WORKING_DIR` variable. I wonder why `/bigtop-packages/src/templates/init.d.tmpl` is not taking care of the creation and deletion of the `$WORKING_DIR` for that?
      Hide
      githubbot ASF GitHub Bot added a comment -

      Github user mxm commented on the pull request:

      https://github.com/apache/bigtop/pull/101#issuecomment-214802211

      >I'm definitively against flink-master, flink-worker, because that's not an "official" terminology (at least in the Flink code we call them job- and taskmanager).

      Then let's call them `flink-jobmanager` and `flink-taskmanager`. The `flink-` prefix will help people to discover the service names.

      Show
      githubbot ASF GitHub Bot added a comment - Github user mxm commented on the pull request: https://github.com/apache/bigtop/pull/101#issuecomment-214802211 >I'm definitively against flink-master, flink-worker, because that's not an "official" terminology (at least in the Flink code we call them job- and taskmanager). Then let's call them `flink-jobmanager` and `flink-taskmanager`. The `flink-` prefix will help people to discover the service names.
      Hide
      githubbot ASF GitHub Bot added a comment -

      Github user rmetzger commented on the pull request:

      https://github.com/apache/bigtop/pull/101#issuecomment-215009947

      Okay, I'll rename the services

      Show
      githubbot ASF GitHub Bot added a comment - Github user rmetzger commented on the pull request: https://github.com/apache/bigtop/pull/101#issuecomment-215009947 Okay, I'll rename the services
      Hide
      githubbot ASF GitHub Bot added a comment -

      Github user rmetzger commented on the pull request:

      https://github.com/apache/bigtop/pull/101#issuecomment-215436502

      I addressed all remaining issues with the pull request.

      Please let me know if there's anything else blocking this from being merged.

      Show
      githubbot ASF GitHub Bot added a comment - Github user rmetzger commented on the pull request: https://github.com/apache/bigtop/pull/101#issuecomment-215436502 I addressed all remaining issues with the pull request. Please let me know if there's anything else blocking this from being merged.
      Hide
      jayunit100 jay vyas added a comment -

      This looks good for a first pass , lets merge.! Konstantin Boudnik YoungWoo Kim Evans Ye Olaf Flebbe

      can we merge https://github.com/apache/bigtop/pull/101.patch

      Show
      jayunit100 jay vyas added a comment - This looks good for a first pass , lets merge.! Konstantin Boudnik YoungWoo Kim Evans Ye Olaf Flebbe can we merge https://github.com/apache/bigtop/pull/101.patch
      Hide
      cos Konstantin Boudnik added a comment -

      A few more...

      • first of
        git apply 101.patch
        101.patch:58: trailing whitespace.
        enable_local_repo: false 
        101.patch:91: trailing whitespace.
        mvn install $FLINK_BUILD_OPTS -Drat.skip=true -DskipTests -Dhadoop.version=$HADOOP_VERSION "$@" 
        101.patch:153: trailing whitespace.
           
        101.patch:443: trailing whitespace.
         
        101.patch:444: trailing whitespace.
         - `recovery.job.delay`: (Default 'akka.ask.timeout') Defines the delay before persisted jobs are recovered in case of a recovery situation. 
        warning: squelched 20 whitespace errors
        warning: 25 lines add whitespace errors.
        
      • bigtop-deploy/vm/vagrant-puppet-vm/vagrantconfig.yaml has irrelevant changes

      RAT report seems to be fine. So let's fix these white-space issues and merge unless there are more comments from others.

      Show
      cos Konstantin Boudnik added a comment - A few more... first of git apply 101.patch 101.patch:58: trailing whitespace. enable_local_repo: false 101.patch:91: trailing whitespace. mvn install $FLINK_BUILD_OPTS -Drat.skip= true -DskipTests -Dhadoop.version=$HADOOP_VERSION "$@" 101.patch:153: trailing whitespace. 101.patch:443: trailing whitespace. 101.patch:444: trailing whitespace. - `recovery.job.delay`: (Default 'akka.ask.timeout') Defines the delay before persisted jobs are recovered in case of a recovery situation. warning: squelched 20 whitespace errors warning: 25 lines add whitespace errors. bigtop-deploy/vm/vagrant-puppet-vm/vagrantconfig.yaml has irrelevant changes RAT report seems to be fine. So let's fix these white-space issues and merge unless there are more comments from others.
      Hide
      rmetzger Robert Metzger added a comment -

      Thank you for the review Konstantin Boudnik.

      I fixed the whitespace errors (btw you can fix them yourself with git apply --whitespace=fix 101.patch) and removed the irrelevant changes.

      Show
      rmetzger Robert Metzger added a comment - Thank you for the review Konstantin Boudnik . I fixed the whitespace errors (btw you can fix them yourself with git apply --whitespace=fix 101.patch ) and removed the irrelevant changes.
      Hide
      cos Konstantin Boudnik added a comment -

      I am pretty sure I can, but then my version will be different from yours and I would need to create a PR, which I don't want to

      Show
      cos Konstantin Boudnik added a comment - I am pretty sure I can, but then my version will be different from yours and I would need to create a PR, which I don't want to
      Hide
      githubbot ASF GitHub Bot added a comment -

      Github user mbalassi commented on the pull request:

      https://github.com/apache/bigtop/pull/101#issuecomment-217156713

      Hey guys, any update on this? I am looking forward to having the Flink integration in.

      Show
      githubbot ASF GitHub Bot added a comment - Github user mbalassi commented on the pull request: https://github.com/apache/bigtop/pull/101#issuecomment-217156713 Hey guys, any update on this? I am looking forward to having the Flink integration in.
      Hide
      cos Konstantin Boudnik added a comment -

      Olaf Flebbe, any other comment from your side?

      Show
      cos Konstantin Boudnik added a comment - Olaf Flebbe , any other comment from your side?
      Hide
      cos Konstantin Boudnik added a comment -

      I have committed this and pushed to the master. Attaching the latest patch for the reference as who know where the GH PRs will be in the future.

      Thanks everybody who helped to get this patch ready!

      Show
      cos Konstantin Boudnik added a comment - I have committed this and pushed to the master. Attaching the latest patch for the reference as who know where the GH PRs will be in the future. Thanks everybody who helped to get this patch ready!
      Hide
      githubbot ASF GitHub Bot added a comment -

      Github user c0s commented on the pull request:

      https://github.com/apache/bigtop/pull/101#issuecomment-217743938

      I have committed this to the master. Feel free to close the PR and thanks for your contribution!

      Show
      githubbot ASF GitHub Bot added a comment - Github user c0s commented on the pull request: https://github.com/apache/bigtop/pull/101#issuecomment-217743938 I have committed this to the master. Feel free to close the PR and thanks for your contribution!
      Hide
      rvs Roman Shaposhnik added a comment -

      Awesome news! Can't wait to deploy Flink as part of my test Bigtop cluster. Btw, any reason it shouldn't be added here: https://ci.bigtop.apache.org/view/Packages/job/Bigtop-trunk-packages/ ?

      Show
      rvs Roman Shaposhnik added a comment - Awesome news! Can't wait to deploy Flink as part of my test Bigtop cluster. Btw, any reason it shouldn't be added here: https://ci.bigtop.apache.org/view/Packages/job/Bigtop-trunk-packages/ ?
      Hide
      cos Konstantin Boudnik added a comment -

      No reason, just no one has done it yet

      Show
      cos Konstantin Boudnik added a comment - No reason, just no one has done it yet
      Hide
      githubbot ASF GitHub Bot added a comment -

      Github user rmetzger commented on the pull request:

      https://github.com/apache/bigtop/pull/101#issuecomment-217852730

      Thanks a lot for merging.
      I'm closing the PR now.

      Show
      githubbot ASF GitHub Bot added a comment - Github user rmetzger commented on the pull request: https://github.com/apache/bigtop/pull/101#issuecomment-217852730 Thanks a lot for merging. I'm closing the PR now.
      Hide
      githubbot ASF GitHub Bot added a comment -

      Github user rmetzger closed the pull request at:

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

      Show
      githubbot ASF GitHub Bot added a comment - Github user rmetzger closed the pull request at: https://github.com/apache/bigtop/pull/101
      Hide
      oflebbe Olaf Flebbe added a comment -

      BTW: I added flink to the bigtop-packages-trunk ci job.

      Show
      oflebbe Olaf Flebbe added a comment - BTW: I added flink to the bigtop-packages-trunk ci job.

        People

        • Assignee:
          Bhupendra Singh Bhupendra Singh
          Reporter:
          Bhupendra Singh Bhupendra Singh
        • Votes:
          0 Vote for this issue
          Watchers:
          10 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development