Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.3.0, 0.4.0
    • Fix Version/s: 0.4.0
    • Component/s: Tests
    • Labels:
      None

      Description

      I plan to attach a patch to make TestJdbcDriver more robust, by parameterizing the wait time for the hive server to be up (it is currently hardcoded to 1 second, which may not be long enough) and by introducing a timeout to the test method (in my experience, if for some reason hive is not up or otherwise able to respond successfully, the test hangs).

      1. BIGTOP-678.txt
        2 kB
        Wing Yew Poon

        Activity

        Hide
        Wing Yew Poon added a comment -

        Patch attached.

        Show
        Wing Yew Poon added a comment - Patch attached.
        Hide
        Wing Yew Poon added a comment -

        I also used "drop table if exists" instead of "drop table".

        Show
        Wing Yew Poon added a comment - I also used "drop table if exists" instead of "drop table".
        Hide
        Bruno Mahé added a comment - - edited

        +1.
        But wouldn't it have been easier to use Integer.getInteger(value, default) and make 1000 and the property name constants?

        Show
        Bruno Mahé added a comment - - edited +1. But wouldn't it have been easier to use Integer.getInteger(value, default) and make 1000 and the property name constants?
        Hide
        Bruno Mahé added a comment -

        Thanks for the patch!

        Show
        Bruno Mahé added a comment - Thanks for the patch!
        Hide
        Wing Yew Poon added a comment - - edited

        Bruno, sure, I was about to upload a new patch with your suggestion to use Integer.getInteger(String property, int default) when you closed this.

        Show
        Wing Yew Poon added a comment - - edited Bruno, sure, I was about to upload a new patch with your suggestion to use Integer.getInteger(String property, int default) when you closed this.
        Hide
        Bruno Mahé added a comment -

        I just reopened the ticket so you can update your patch

        Show
        Bruno Mahé added a comment - I just reopened the ticket so you can update your patch
        Hide
        Wing Yew Poon added a comment -

        Bruno, on second thoughts, I think I'd leave it the way it is for now. Using Integer.getInteger(String, int) would indeed be easier. However, using System.getProperty(String) makes it easier later to find all the properties used by the tests. Right now, we don't have a way in the tests to specify a contract, in particular, what are the inputs (possibly optional) to a test, which typically is done by setting system properties. Until we have some kind of annotation or other way (even just a convention) for specifying such a contract, I'd leave the property in a System.getProperty(String) call.

        Show
        Wing Yew Poon added a comment - Bruno, on second thoughts, I think I'd leave it the way it is for now. Using Integer.getInteger(String, int) would indeed be easier. However, using System.getProperty(String) makes it easier later to find all the properties used by the tests. Right now, we don't have a way in the tests to specify a contract, in particular, what are the inputs (possibly optional) to a test, which typically is done by setting system properties. Until we have some kind of annotation or other way (even just a convention) for specifying such a contract, I'd leave the property in a System.getProperty(String) call.
        Hide
        Bruno Mahé added a comment -

        What about wrapping it in a util function or something similar? If we get this sort of code all over the place, it is not going to be very easy to follow.

        Show
        Bruno Mahé added a comment - What about wrapping it in a util function or something similar? If we get this sort of code all over the place, it is not going to be very easy to follow.
        Hide
        Wing Yew Poon added a comment -

        What I meant was that, afaik, tests that use or depend on system properties or environment variables to parameterize behavior contain System.getProperty() or System.getenv() calls, so we can currently determine what the set of properties and variables are by grepping for System.getProperty and System.getenv in the test code. If instead, I start using Integer.getInteger(), then that would be one more (less obvious) thing to keep track of to grep for. You can imagine at least writing a tool (script) to come up with the "contract" (set of properties and variables) even in the current imperfect state of things. Changing the code to use Integer.getInteger() makes writing this tool a little more complex.

        Show
        Wing Yew Poon added a comment - What I meant was that, afaik, tests that use or depend on system properties or environment variables to parameterize behavior contain System.getProperty() or System.getenv() calls, so we can currently determine what the set of properties and variables are by grepping for System.getProperty and System.getenv in the test code. If instead, I start using Integer.getInteger(), then that would be one more (less obvious) thing to keep track of to grep for. You can imagine at least writing a tool (script) to come up with the "contract" (set of properties and variables) even in the current imperfect state of things. Changing the code to use Integer.getInteger() makes writing this tool a little more complex.
        Hide
        Bruno Mahé added a comment -

        What I meant was that, being able to grep on the properties being used is fair.
        But if the cost of it is to have such template copy/pasted in such form all over the place, I am not sure I will go along. It's fine for a one time patch, but wouldn't work if applied to the whole code base.
        Hence my suggestion to refactor it or to wrap it up in a util function you could grep on as well.

        Show
        Bruno Mahé added a comment - What I meant was that, being able to grep on the properties being used is fair. But if the cost of it is to have such template copy/pasted in such form all over the place, I am not sure I will go along. It's fine for a one time patch, but wouldn't work if applied to the whole code base. Hence my suggestion to refactor it or to wrap it up in a util function you could grep on as well.
        Hide
        Wing Yew Poon added a comment -

        Bruno, I'd like this issue to be closed. I am addressing the issue of how to deal with system properties elsewhere, in BIGTOP-685.

        Show
        Wing Yew Poon added a comment - Bruno, I'd like this issue to be closed. I am addressing the issue of how to deal with system properties elsewhere, in BIGTOP-685 .

          People

          • Assignee:
            Wing Yew Poon
            Reporter:
            Wing Yew Poon
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development