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

Return value does not reflect status checks

    Details

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

      Description

      The init script for HBase RegionServers that supports multiple processes is always returning 0. It should return 0 on success of non-zero on failure. In the case of running multiple processes, failure is defined as a failure

      There are already different constants defined in the file (e.g. different values for failures in all process, failures in some processes, failures in no processes... etc.), we just need to return them properly.

      Although this affects the 0.7.0 RC, I don't consider it a big enough deal to warrant -1'ing the RC, which I'm still testing.

        Activity

        Hide
        mgrover Mark Grover added a comment -

        That's fine by me.
        +1 and thanks!

        Show
        mgrover Mark Grover added a comment - That's fine by me. +1 and thanks!
        Hide
        mackrorysd Sean Mackrory added a comment -

        As I don't consider any of that to be a regression or existing bug since we previously would just return success unconditionally, how about we defer that improvement to a separate issue (just created https://issues.apache.org/jira/browse/BIGTOP-1127) so that the current patch that does currently implement correct behavior can go in without another round of testing and review?

        Show
        mackrorysd Sean Mackrory added a comment - As I don't consider any of that to be a regression or existing bug since we previously would just return success unconditionally, how about we defer that improvement to a separate issue (just created https://issues.apache.org/jira/browse/BIGTOP-1127 ) so that the current patch that does currently implement correct behavior can go in without another round of testing and review?
        Hide
        mgrover Mark Grover added a comment -

        Thanks Sean! It's looking good!

        If you'd allow me to pick out another nit. In the case statement at the very bottom, we are still picking up the return code implicitly. What do you think about perhaps, explicitly collecting the code returned by each of the functions (start, stop, etc.) in a new variable and return that variable in the end instead of $?

        That might make the code less fragile. Consider the scenario where someone comes and adds an echo statement right after the call to restart(), that would erroneously change the return code to always be true, no?

        Show
        mgrover Mark Grover added a comment - Thanks Sean! It's looking good! If you'd allow me to pick out another nit. In the case statement at the very bottom, we are still picking up the return code implicitly. What do you think about perhaps, explicitly collecting the code returned by each of the functions (start, stop, etc.) in a new variable and return that variable in the end instead of $? That might make the code less fragile. Consider the scenario where someone comes and adds an echo statement right after the call to restart(), that would erroneously change the return code to always be true, no?
        Hide
        mackrorysd Sean Mackrory added a comment -

        Good point, Mark, although the return value was going to be 0 with or without my patch. I've attached a patch that uses 'return' instead of 'exit' more consistently and returns values that better represent the status, and I've also attached a transcript of my test sequence, which I have read through and verified that the output was always what I thought it should be.

        I have only made changes for the single-process use case. The function for starting and stopping multiple daemons also returns 0 unconditionally, as I recall there being a reason for that, so perhaps lets wait on adding any extra complexity to that function. Any users of the multiple-process feature would need to query the status to know which had failed, anyway.

        Show
        mackrorysd Sean Mackrory added a comment - Good point, Mark, although the return value was going to be 0 with or without my patch. I've attached a patch that uses 'return' instead of 'exit' more consistently and returns values that better represent the status, and I've also attached a transcript of my test sequence, which I have read through and verified that the output was always what I thought it should be. I have only made changes for the single-process use case. The function for starting and stopping multiple daemons also returns 0 unconditionally, as I recall there being a reason for that, so perhaps lets wait on adding any extra complexity to that function. Any users of the multiple-process feature would need to query the status to know which had failed, anyway.
        Hide
        mgrover Mark Grover added a comment -

        Thanks Sean.

        I am happy with the typo correction (good find!). However, I am a little concerned about changing the exit code of the script at the bottom. Even though I believe that's the right thing to do, I think it may warrant a little more testing.

        While I haven't tried this myself but in case stop() returns an error, the last line in that method is echo "ERROR: " (reference)

        This would return 0 from the function which means the script returns success even though it was an error (reference).

        Am I understanding it correct? Please let me know what you think!

        Show
        mgrover Mark Grover added a comment - Thanks Sean. I am happy with the typo correction (good find!). However, I am a little concerned about changing the exit code of the script at the bottom. Even though I believe that's the right thing to do, I think it may warrant a little more testing. While I haven't tried this myself but in case stop() returns an error, the last line in that method is echo "ERROR: " ( reference ) This would return 0 from the function which means the script returns success even though it was an error ( reference ). Am I understanding it correct? Please let me know what you think!
        Hide
        rvs Roman Shaposhnik added a comment -

        Thanks, Sean! I'll tag it with 0.8.0 then.

        Show
        rvs Roman Shaposhnik added a comment - Thanks, Sean! I'll tag it with 0.8.0 then.
        Hide
        mackrorysd Sean Mackrory added a comment -

        I've tested the start, stop, restart and status commands for a single process. I've also tested these operations with multiple operations, and all looks good. Note that if you run the status command and multiple processes are running, it will (correctly, IMO) report success as long as the processes it can find a trace of are running. It will report failure if you specify a process that has previously been shut down.

        Show
        mackrorysd Sean Mackrory added a comment - I've tested the start, stop, restart and status commands for a single process. I've also tested these operations with multiple operations, and all looks good. Note that if you run the status command and multiple processes are running, it will (correctly, IMO) report success as long as the processes it can find a trace of are running. It will report failure if you specify a process that has previously been shut down.

          People

          • Assignee:
            mackrorysd Sean Mackrory
            Reporter:
            mackrorysd Sean Mackrory
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development