Uploaded image for project: 'Bigtop'
  1. Bigtop
  2. BIGTOP-1337

Logging success messages in service startup and shutdown only after verification.

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.8.0
    • Fix Version/s: 0.8.0
    • Component/s: None
    • Labels:

      Description

      Right now, it appears like we log success messages right at the beginning of the method call irrespective of whether the service was successfully stopped / started. In a scenario where the user issues "service serviceName start" and immediately sees a success message but, the service in reality has failed to start leads to some amount of confusion.

      The proposed change will only log success messages if the operation was actually successful.

      1. BIGTOP-1337.patch
        9 kB
        Manikandan Narayanaswamy

        Activity

        Hide
        mnarayan Manikandan Narayanaswamy added a comment - - edited

        Uploading patch after testing changes on CentOS and Ubuntu.

        Testing done:
        Installed spark-history-server on Ubuntu and CentOS.

        • Saw that we now log failure if the service fails to start.
        • Saw success message only if the service actually started.

        The changes are identical in other init scripts so did not verify all of them.

        Show
        mnarayan Manikandan Narayanaswamy added a comment - - edited Uploading patch after testing changes on CentOS and Ubuntu. Testing done: Installed spark-history-server on Ubuntu and CentOS. Saw that we now log failure if the service fails to start. Saw success message only if the service actually started. The changes are identical in other init scripts so did not verify all of them.
        Hide
        plinnell Peter Linnell added a comment -

        Seems the patch is missing.

        Show
        plinnell Peter Linnell added a comment - Seems the patch is missing.
        Hide
        mnarayan Manikandan Narayanaswamy added a comment -

        Missed adding the patch earlier, attaching now. Thanks Peter.

        Show
        mnarayan Manikandan Narayanaswamy added a comment - Missed adding the patch earlier, attaching now. Thanks Peter.
        Hide
        rvs Roman Shaposhnik added a comment -

        Manikandan Narayanaswamy thanks for the patch. The way it is posted it doesn't seem to apply. It tries to patch Impala and Bigtop doesn't package it. Also the patch seems to be touching:

        error: bigtop-packages/src/common/hadoop/hadoop-hdfs-nfs3.svc: does not exist in index
        error: bigtop-packages/src/common/hbase-solr/hbase-solr-indexer.svc: does not exist in index
        error: bigtop-packages/src/common/sentry/sentry-store.svc: does not exist in index
        

        Is there any chance you can backport the missing packaging functionality as well before applying this patch?

        Feel free to file extra JIRAs for Hadoop NFS, Sentry store and whatever else is missing.

        Show
        rvs Roman Shaposhnik added a comment - Manikandan Narayanaswamy thanks for the patch. The way it is posted it doesn't seem to apply. It tries to patch Impala and Bigtop doesn't package it. Also the patch seems to be touching: error: bigtop-packages/src/common/hadoop/hadoop-hdfs-nfs3.svc: does not exist in index error: bigtop-packages/src/common/hbase-solr/hbase-solr-indexer.svc: does not exist in index error: bigtop-packages/src/common/sentry/sentry-store.svc: does not exist in index Is there any chance you can backport the missing packaging functionality as well before applying this patch? Feel free to file extra JIRAs for Hadoop NFS, Sentry store and whatever else is missing.
        Hide
        mnarayan Manikandan Narayanaswamy added a comment -

        Updated patch to only include files that are present in BIGTOP.

        Show
        mnarayan Manikandan Narayanaswamy added a comment - Updated patch to only include files that are present in BIGTOP.
        Hide
        mnarayan Manikandan Narayanaswamy added a comment -

        Thanks Roman. I just got back from a vacation and hence the delay.

        I've removed the modified files that currently are not present in BIGTOP. I've applied the patch to my local BIGTOP repo and found that it worked fine. Could you please take a look at the patch now.

        I'll file a separate JIRA for backporting the changes that are not in BIGTOP.

        Show
        mnarayan Manikandan Narayanaswamy added a comment - Thanks Roman. I just got back from a vacation and hence the delay. I've removed the modified files that currently are not present in BIGTOP. I've applied the patch to my local BIGTOP repo and found that it worked fine. Could you please take a look at the patch now. I'll file a separate JIRA for backporting the changes that are not in BIGTOP.
        Hide
        mackrorysd Sean Mackrory added a comment -

        Congratulations on getting BIGTOP-1337, Mani. Just some context on the missing functionality and it's current status in Bigtop:

        BIGTOP-1362 exists to package hadoop-hdfs-ndfs service and is almost done

        I believe the Sentry Store service is not yet included in an upstream sentry release, and until now Apache Sentry (incubating) has just been a bunch of JARs that other projects can include and integrate with - so until Sentry Store, Sentry has not been an area where Bigtop can add much value (projects like Tez and Kite have been in a similar boat and haven't been adopted into Bigtop very quickly for similar reasons).

        Show
        mackrorysd Sean Mackrory added a comment - Congratulations on getting BIGTOP-1337 , Mani. Just some context on the missing functionality and it's current status in Bigtop: BIGTOP-1362 exists to package hadoop-hdfs-ndfs service and is almost done I believe the Sentry Store service is not yet included in an upstream sentry release, and until now Apache Sentry (incubating) has just been a bunch of JARs that other projects can include and integrate with - so until Sentry Store, Sentry has not been an area where Bigtop can add much value (projects like Tez and Kite have been in a similar boat and haven't been adopted into Bigtop very quickly for similar reasons).
        Hide
        mackrorysd Sean Mackrory added a comment -

        +1 on the patch, by the way. I'm happy to commit it, but could you first re-submit it with the Bigtop JIRA number and description as the subject? It is also CDH-specific right now.

        Show
        mackrorysd Sean Mackrory added a comment - +1 on the patch, by the way. I'm happy to commit it, but could you first re-submit it with the Bigtop JIRA number and description as the subject? It is also CDH-specific right now.
        Hide
        mackrorysd Sean Mackrory added a comment -

        Re-assigned and committed (assuming that's okay with Roman Shaposhnik). By the way - it is customary to leave previous attachments in place so that others can see what the latest patch was at the time previous comments were made, and can see the changes as the patch evolved.

        Show
        mackrorysd Sean Mackrory added a comment - Re-assigned and committed (assuming that's okay with Roman Shaposhnik ). By the way - it is customary to leave previous attachments in place so that others can see what the latest patch was at the time previous comments were made, and can see the changes as the patch evolved.
        Hide
        rvs Roman Shaposhnik added a comment -

        Sean Mackrory totally cool with me! Thanks for looking into this.

        Show
        rvs Roman Shaposhnik added a comment - Sean Mackrory totally cool with me! Thanks for looking into this.
        Hide
        mnarayan Manikandan Narayanaswamy added a comment -

        Thanks Sean, Roman!

        Show
        mnarayan Manikandan Narayanaswamy added a comment - Thanks Sean, Roman!

          People

          • Assignee:
            mnarayan Manikandan Narayanaswamy
            Reporter:
            mnarayan Manikandan Narayanaswamy
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development