Bigtop
  1. Bigtop
  2. BIGTOP-835

The shell exec method must have variants which have timeout and can run in background

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.6.0
    • Component/s: None
    • Labels:
      None
    1. BIGTOP-835-4-formatted.patch
      8 kB
      Hari Shreedharan
    2. BIGTOP-835-4.patch
      8 kB
      Hari Shreedharan
    3. BIGTOP-835-3.patch
      8 kB
      Hari Shreedharan
    4. BIGTOP-835-2.patch
      8 kB
      Hari Shreedharan
    5. BIGTOP-835-1.patch
      8 kB
      Hari Shreedharan

      Issue Links

        Activity

        Hide
        Roman Shaposhnik added a comment -

        That would be useful indeed. The only potential concern is traceability since one would have to figure out how to interleave the stdout/stderr output streams. That said, perhaps we can say that you don't get traceability when you use shell in those modes.

        Show
        Roman Shaposhnik added a comment - That would be useful indeed. The only potential concern is traceability since one would have to figure out how to interleave the stdout/stderr output streams. That said, perhaps we can say that you don't get traceability when you use shell in those modes.
        Hide
        Hari Shreedharan added a comment -

        I am working on a patch for this. Right now we do have 2 streams in the class, I am planning to just do the same for this as well. I will submit the patch soon - then we can discuss any specific documentation we need to add?

        Show
        Hari Shreedharan added a comment - I am working on a patch for this. Right now we do have 2 streams in the class, I am planning to just do the same for this as well. I will submit the patch soon - then we can discuss any specific documentation we need to add?
        Hide
        Hari Shreedharan added a comment -

        Patch available for review

        Show
        Hari Shreedharan added a comment - Patch available for review
        Hide
        Konstantin Boudnik added a comment - - edited

        I think this is a misconception: exec doesn't run in background. Exec in background should be called fork. Please do not confuse the semantics. E.g. -1 in the way the patch is done right now.

        Show
        Konstantin Boudnik added a comment - - edited I think this is a misconception: exec doesn't run in background. Exec in background should be called fork . Please do not confuse the semantics. E.g. -1 in the way the patch is done right now.
        Hide
        Johnny Zhang added a comment -

        I think most of the tests in Bigtop uses @Test(timeout=???L) to control the timeout of the testcase. In this case, execWithTimeout seems redundant for me. Plus, it will be equipment only if it can print meaningful error message.

        I also agree with cos that introducing those methods kind of confuse the concepts. Maybe we can move those implementation to another class. Also, more error message will be helpful.

        Show
        Johnny Zhang added a comment - I think most of the tests in Bigtop uses @Test(timeout=???L) to control the timeout of the testcase. In this case, execWithTimeout seems redundant for me. Plus, it will be equipment only if it can print meaningful error message. I also agree with cos that introducing those methods kind of confuse the concepts. Maybe we can move those implementation to another class. Also, more error message will be helpful.
        Hide
        Konstantin Boudnik added a comment -

        if you will just name you patches uniformly - like ID.patch - JIRA will automatically take care about the sequence.

        Show
        Konstantin Boudnik added a comment - if you will just name you patches uniformly - like ID.patch - JIRA will automatically take care about the sequence.
        Hide
        Konstantin Boudnik added a comment -

        and approach is still confusing exec semantic.

        Show
        Konstantin Boudnik added a comment - and approach is still confusing exec semantic.
        Hide
        Hari Shreedharan added a comment - - edited

        Johnny Zhang - Here is my use-case:
        Apache Flume is a long running process, it will wait for input as long as it is not killed. So in writing some tests for Flume, I hit a situation where exec from iTest was pretty much useless to me. The case where I just needed to wait for a self-running source to generate the data, I could use the execWithTimeout and in the other case where I started the process and then write data via Avro RPC to Flume, I used the equivalent of execInBackground. Both these cases cannot be solved with the @Test(timeout).

        Konstantin Boudnik - If you think the execInBackground should be called fork, I am ok with that and can do it (That's the only thing I understood from your feedback - if there is something else, please let me know). I don't intend to change much of the code (except maybe renaming the methods), as I do not have the cycles to do much follow up. I was just contributing what I already did.

        Also, I don't think Bigtop has a policy on naming of patches, does it? I was just following the convention I use in other projects.

        Show
        Hari Shreedharan added a comment - - edited Johnny Zhang - Here is my use-case: Apache Flume is a long running process, it will wait for input as long as it is not killed. So in writing some tests for Flume, I hit a situation where exec from iTest was pretty much useless to me. The case where I just needed to wait for a self-running source to generate the data, I could use the execWithTimeout and in the other case where I started the process and then write data via Avro RPC to Flume, I used the equivalent of execInBackground. Both these cases cannot be solved with the @Test(timeout). Konstantin Boudnik - If you think the execInBackground should be called fork, I am ok with that and can do it (That's the only thing I understood from your feedback - if there is something else, please let me know). I don't intend to change much of the code (except maybe renaming the methods), as I do not have the cycles to do much follow up. I was just contributing what I already did. Also, I don't think Bigtop has a policy on naming of patches, does it? I was just following the convention I use in other projects.
        Hide
        Johnny Zhang added a comment - - edited

        Hari Shreedharan, thanks for explaining this, your use case strongly justify the needs. It is actually like what I met in Mahout testing: the data mining algorithm takes long long time to generate the data. However, my test itself just need to make sure Mahout finish the job, it won't check the results by comparing with any golden standard.

        The only concern for me is what if test trigger a MR job and it hangs? It will hold all the other MR jobs in the queue forever until you manually kill it. Since no error message, or other trace, it is hard to debug the test failure/hanging. This is what exactly I met in Mahout testing (using @Test(timeout) cannot handle it well since you don't know how long it should take). If this is the case, how about adding a block of self cleaning in the end? For example, execute the process without timeout or in background, but check if the job hangs or doesn't fail gracefully in the end, if so, kill the MR job.

        Show
        Johnny Zhang added a comment - - edited Hari Shreedharan , thanks for explaining this, your use case strongly justify the needs. It is actually like what I met in Mahout testing: the data mining algorithm takes long long time to generate the data. However, my test itself just need to make sure Mahout finish the job, it won't check the results by comparing with any golden standard. The only concern for me is what if test trigger a MR job and it hangs? It will hold all the other MR jobs in the queue forever until you manually kill it. Since no error message, or other trace, it is hard to debug the test failure/hanging. This is what exactly I met in Mahout testing (using @Test(timeout) cannot handle it well since you don't know how long it should take). If this is the case, how about adding a block of self cleaning in the end? For example, execute the process without timeout or in background, but check if the job hangs or doesn't fail gracefully in the end, if so, kill the MR job.
        Hide
        Hari Shreedharan added a comment -

        I am not sure how we can handle process failure or process hangs. We could just set a non-zero return code, and expect the client code to clean up. I don't think this makes anything worse than it is now, does it? Even now the exec method will not clean up by killing the mr job right (using @Test (timeout))?

        Show
        Hari Shreedharan added a comment - I am not sure how we can handle process failure or process hangs. We could just set a non-zero return code, and expect the client code to clean up. I don't think this makes anything worse than it is now, does it? Even now the exec method will not clean up by killing the mr job right (using @Test (timeout))?
        Hide
        Konstantin Boudnik added a comment -

        Also, I don't think Bigtop has a policy on naming of patches,

        it isn't about policy - it is about using JIRA mechanism that doesn't force anyone to understand how you number your patches Just a courtesy, perhaps?

        Show
        Konstantin Boudnik added a comment - Also, I don't think Bigtop has a policy on naming of patches, it isn't about policy - it is about using JIRA mechanism that doesn't force anyone to understand how you number your patches Just a courtesy, perhaps?
        Hide
        Johnny Zhang added a comment -

        Hari Shreedharan, you are right about it. @Test (timeout) cannot handle it either.

        +1 on your patch.

        Show
        Johnny Zhang added a comment - Hari Shreedharan , you are right about it. @Test (timeout) cannot handle it either. +1 on your patch.
        Hide
        Mark Grover added a comment -

        Barring Cos' suggestions about rename, etc., I am +1 on this too.

        Show
        Mark Grover added a comment - Barring Cos' suggestions about rename, etc., I am +1 on this too.
        Hide
        Hari Shreedharan added a comment -

        Konstantin Boudnik Do you have any concerns other than the exeInBackground method being renamed to fork ? If not, I will submit a patch that makes this change.

        Show
        Hari Shreedharan added a comment - Konstantin Boudnik Do you have any concerns other than the exeInBackground method being renamed to fork ? If not, I will submit a patch that makes this change.
        Hide
        Konstantin Boudnik added a comment - - edited

        yes, as far as you do not redefine the semantics of exec - go ahead with it.

        Show
        Konstantin Boudnik added a comment - - edited yes, as far as you do not redefine the semantics of exec - go ahead with it.
        Hide
        Hari Shreedharan added a comment - - edited

        Renamed execInBackground to fork. For consistency, naming the patches as before.

        Show
        Hari Shreedharan added a comment - - edited Renamed execInBackground to fork. For consistency, naming the patches as before.
        Hide
        Mark Grover added a comment -

        +1 (non-committer)

        Show
        Mark Grover added a comment - +1 (non-committer)
        Hide
        Roman Shaposhnik added a comment -

        +1. Thanks a million for a patch, Hari! I'll be committing this soon.

        Show
        Roman Shaposhnik added a comment - +1. Thanks a million for a patch, Hari! I'll be committing this soon.
        Hide
        Hari Shreedharan added a comment - - edited

        Thanks Roman. This is the git formatted patch (BIGTOP-835-4-formatted.patch).

        Show
        Hari Shreedharan added a comment - - edited Thanks Roman. This is the git formatted patch ( BIGTOP-835 -4-formatted.patch).
        Hide
        Roman Shaposhnik added a comment -

        Committed to master

        Show
        Roman Shaposhnik added a comment - Committed to master
        Hide
        Hari Shreedharan added a comment -

        I thought this one was closed?

        Show
        Hari Shreedharan added a comment - I thought this one was closed?
        Hide
        Roman Shaposhnik added a comment -

        We typically bulk-close at the time of the release. But yes – it is in.

        Show
        Roman Shaposhnik added a comment - We typically bulk-close at the time of the release. But yes – it is in.

          People

          • Assignee:
            Roman Shaposhnik
            Reporter:
            Hari Shreedharan
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development