Kafka
  1. Kafka
  2. KAFKA-786

Use "withRequiredArg" while parsing jopt options in all tools

    Details

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

      Description

      While parsing jopt Options in our tools, we sometimes use withRequiredArg() and sometimes use withOptionalArg(). I think this confusing and we should always use withRequiredArg().

      withOptionalArg() allows you to provide an option without an argument. For instance, the following commands will yield the same result if xyz was a parser option that accepted an optional argument and was provided a default in the tool's code:
      kafka-tool.sh --xyz
      kafka-tool.sh

      I don't quite see the need to allow the 1st command, think that writing code will be less confusing if we allowed only the second command. To do that, we can make all options require arguments. These arguments will need to be given via command line or via a default in the code. So if xyz was an option that required an argument then you will see the following:

      kafka-tool.sh --xyz
      Option ['xyz'] requires an argument //printed by jOpt

      kafka-tool.sh --xyz argumentVal
      // Kafka tool proceeds

      If you want to use a default value specified for xyz in the code, then simply run ./kafka-tool.sh.

      1. kafka-786.patch
        5 kB
        Swapnil Ghike
      2. kafka-786-v2.patch
        5 kB
        Swapnil Ghike

        Activity

        Swapnil Ghike created issue -
        Hide
        Swapnil Ghike added a comment - - edited

        Also note that withRequiredArg() does not check if xyz has a non-null value, we still need to perform that check ourselves in the code. So, jopt will do nothing to prevent you from running into a NPE, if you ran
        kafka.tool.sh and xyz was an option that required an argument, but was not provided a default value in the code.

        Show
        Swapnil Ghike added a comment - - edited Also note that withRequiredArg() does not check if xyz has a non-null value, we still need to perform that check ourselves in the code. So, jopt will do nothing to prevent you from running into a NPE, if you ran kafka.tool.sh and xyz was an option that required an argument, but was not provided a default value in the code.
        Hide
        Swapnil Ghike added a comment -

        Attached a patch.

        Show
        Swapnil Ghike added a comment - Attached a patch.
        Swapnil Ghike made changes -
        Field Original Value New Value
        Attachment kafka-786.patch [ 12572008 ]
        Swapnil Ghike made changes -
        Assignee Swapnil Ghike [ swapnilghike ]
        Hide
        Swapnil Ghike added a comment -

        Jun raised a good question: What do we do for options like "print" or "verify" or "enable" etc? These options typically don't expect a boolean argument, it should be enough to specify these options on the command line to sort of enable them in the program.

        The answer is to not use any of withRequiredArg() or withOptionalArg(). We can simply write something like the following:
        val printOpt = parser.accepts("print-data-log", "if set, printing the messages content when dumping data logs")

        Later we should check in the program like the following:
        if(options.has(printOpt) print() else doNothing()

        Show
        Swapnil Ghike added a comment - Jun raised a good question: What do we do for options like "print" or "verify" or "enable" etc? These options typically don't expect a boolean argument, it should be enough to specify these options on the command line to sort of enable them in the program. The answer is to not use any of withRequiredArg() or withOptionalArg(). We can simply write something like the following: val printOpt = parser.accepts("print-data-log", "if set, printing the messages content when dumping data logs") Later we should check in the program like the following: if(options.has(printOpt) print() else doNothing()
        Swapnil Ghike made changes -
        Attachment kafka-786-v2.patch [ 12572205 ]
        Hide
        Jun Rao added a comment -

        Thanks for the patch. Committed to 0.8.

        Show
        Jun Rao added a comment - Thanks for the patch. Committed to 0.8.
        Jun Rao made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 0.8 [ 12317244 ]
        Resolution Fixed [ 1 ]
        Neha Narkhede made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Swapnil Ghike
            Reporter:
            Swapnil Ghike
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development