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

Prioritization is broken in bigtop-detect-javahome

    Details

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

      Description

      bigtop-detect-javahome uses 'ls -rd' to sort the JDKs in each category in reverse lexicographical order, but I believe when we switched to having separate variables defined for each major JDK version we overrode that mechanism so older JDKs still take precedence over newer ones.

        Issue Links

          Activity

          Hide
          cos Konstantin Boudnik added a comment -

          Great catch, Sean - thanks!

          Show
          cos Konstantin Boudnik added a comment - Great catch, Sean - thanks!
          Hide
          rvs Roman Shaposhnik added a comment -

          Is there a patch?

          Show
          rvs Roman Shaposhnik added a comment - Is there a patch?
          Hide
          mackrorysd Sean Mackrory added a comment -

          I think the only way to iterate through regular expressions without expanding them via bash (instead of waiting until we can use ls -rd to do it like we want to) is to store them in arrays and then do some magic to make sure they all stay 1-dimensional until we iterate through the final list. Patch attached... tested fairly exhaustively - even on bash on SLES 11 and RHEL 5 in case some array features were newish - but please do review judiciously

          Show
          mackrorysd Sean Mackrory added a comment - I think the only way to iterate through regular expressions without expanding them via bash (instead of waiting until we can use ls -rd to do it like we want to) is to store them in arrays and then do some magic to make sure they all stay 1-dimensional until we iterate through the final list. Patch attached... tested fairly exhaustively - even on bash on SLES 11 and RHEL 5 in case some array features were newish - but please do review judiciously
          Hide
          rvs Roman Shaposhnik added a comment -

          +1. Sean Mackrory youda man!

          Show
          rvs Roman Shaposhnik added a comment - +1. Sean Mackrory youda man!
          Hide
          jayunit100 jay vyas added a comment -

          are we commiting this ? its important to have working JAVA_HOME >
          i didnt test yet, but looks like a great patch, +1 please commit!

          Show
          jayunit100 jay vyas added a comment - are we commiting this ? its important to have working JAVA_HOME > i didnt test yet, but looks like a great patch, +1 please commit!
          Hide
          mackrorysd Sean Mackrory added a comment -

          Comitted and pushed

          Show
          mackrorysd Sean Mackrory added a comment - Comitted and pushed
          Hide
          mackrorysd Sean Mackrory added a comment -

          So I found some issues the other day - you need a correct combination of ordering between the different regexes and between different matches of the regexes, etc. and I just don't think you can get the wildcards to be evaluated at only the right time. So I'm attaching a second attempt. I've tested it with multiple versions of Oracle JDK 1.7 and multiple major versions of OpenJDK installed and this finally got all of them in the right order at once.

          Given how sensitive this is to any kind of gap in testing, I think it'd be great to actually have this tried out by more people with various JDKs on various distros before we try changing this again, rather than just a +1.

          Show
          mackrorysd Sean Mackrory added a comment - So I found some issues the other day - you need a correct combination of ordering between the different regexes and between different matches of the regexes, etc. and I just don't think you can get the wildcards to be evaluated at only the right time. So I'm attaching a second attempt. I've tested it with multiple versions of Oracle JDK 1.7 and multiple major versions of OpenJDK installed and this finally got all of them in the right order at once. Given how sensitive this is to any kind of gap in testing, I think it'd be great to actually have this tried out by more people with various JDKs on various distros before we try changing this again, rather than just a +1.
          Hide
          mgrover Mark Grover added a comment -

          +1 on the latest patch. Thanks Sean!

          Given the number of times this has been broken, I think it's time to re-write this. I really don't think bash is the right language to do this and I think something like Python would be a great fit. What do folks think?

          Show
          mgrover Mark Grover added a comment - +1 on the latest patch. Thanks Sean! Given the number of times this has been broken, I think it's time to re-write this. I really don't think bash is the right language to do this and I think something like Python would be a great fit. What do folks think?
          Hide
          mackrorysd Sean Mackrory added a comment -

          Thanks, Mark. I like the idea of redoing this in a more strongly typed language. Much of the problem has been the trickiness of managing nested lists and controlling when wildcards get evaluated.

          Show
          mackrorysd Sean Mackrory added a comment - Thanks, Mark. I like the idea of redoing this in a more strongly typed language. Much of the problem has been the trickiness of managing nested lists and controlling when wildcards get evaluated.
          Hide
          cos Konstantin Boudnik added a comment -

          -1 on Python as it will cause a compatibility mess - we've discussed it before I guess. beside Bigtop is coming packaged with Groovy runtime, so why add more complexities and dependencies?

          Show
          cos Konstantin Boudnik added a comment - -1 on Python as it will cause a compatibility mess - we've discussed it before I guess. beside Bigtop is coming packaged with Groovy runtime, so why add more complexities and dependencies?
          Hide
          mgrover Mark Grover added a comment -

          Thanks Sean. I will create a JIRA to re-write this.

          Cos, can you please provide more details about the compatibility mess you are referring to? At the end of the day, I think it's simply a language choice so I don't want to elongate this argument and it seems to be a nice clean scripting way of doing this. I don't see why a language choice would garner such a strong reaction but perhaps there is something I am missing

          In any case, I will file a JIRA for the re-write.

          Show
          mgrover Mark Grover added a comment - Thanks Sean. I will create a JIRA to re-write this. Cos, can you please provide more details about the compatibility mess you are referring to? At the end of the day, I think it's simply a language choice so I don't want to elongate this argument and it seems to be a nice clean scripting way of doing this. I don't see why a language choice would garner such a strong reaction but perhaps there is something I am missing In any case, I will file a JIRA for the re-write.
          Hide
          cos Konstantin Boudnik added a comment -

          Cos, can you please provide more details about the compatibility mess you are referring to?

          I am talking about issues where code works with this version of Python and not that version of it; libs getting changed, etc. I am no expert on that language, mostly because of the horror stories I keep seeing on the inet and elsewhere.

          I think you misread me - there's no strong reaction: just a question why it's a good idea to introduce something they might be broken if target system has an incompatible version of the environment? But let's take this discussion to the new JIRA, I guess

          Show
          cos Konstantin Boudnik added a comment - Cos, can you please provide more details about the compatibility mess you are referring to? I am talking about issues where code works with this version of Python and not that version of it; libs getting changed, etc. I am no expert on that language, mostly because of the horror stories I keep seeing on the inet and elsewhere. I think you misread me - there's no strong reaction: just a question why it's a good idea to introduce something they might be broken if target system has an incompatible version of the environment? But let's take this discussion to the new JIRA, I guess
          Hide
          cos Konstantin Boudnik added a comment -

          Seems to get pushed. Shall the ticket be resolved?

          Show
          cos Konstantin Boudnik added a comment - Seems to get pushed. Shall the ticket be resolved?
          Hide
          mackrorysd Sean Mackrory added a comment -

          Oh yes - thanks for reminding me. I'd say this is resolved. Discussion on a reimplementation on not-BASH can be on BIGTOP-1704

          Show
          mackrorysd Sean Mackrory added a comment - Oh yes - thanks for reminding me. I'd say this is resolved. Discussion on a reimplementation on not-BASH can be on BIGTOP-1704

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development