jepsen-io / maelstrom

A workbench for writing toy implementations of distributed systems.
Eclipse Public License 1.0
3.03k stars 188 forks source link

Default workload name not being set when not explicitly passed #78

Closed renancloudwalk closed 1 year ago

renancloudwalk commented 1 year ago

Found a minor issue when running maelstrom against a binary without explicitly setting the workload -w option, the test throws a NullPointerException.

Probably the workload set here

(def test-opt-spec
  "Options for single tests."
  [[nil "--bin FILE"        "Path to binary which runs a node"
    :missing "Expected a --bin PATH_TO_BINARY to test"]

   ["-w" "--workload NAME" "What workload to run."
    :default "lin-kv"
    :parse-fn keyword
    :validate [workloads (cli/one-of workloads)]]
   ])

Is not setting the workload-name here

        workload-name (:workload opts)
        workload      ((workloads workload-name)
                       (assoc opts :nodes nodes, :net net))

Reproduction Steps:

Execute the command: ./maelstrom test --bin demobin the error message which includes the main details:

ERROR [2023-09-09 21:22:36,041] main - jepsen.cli Oh jeez, I'm sorry, Jepsen broke. Here's why:
java.lang.NullPointerException: Cannot invoke "clojure.lang.IFn.invoke(Object)" because the return va        at maelstrom.core$maelstrom_test.invokeStatic(core.clj:62)
        at maelstrom.core$maelstrom_test.invoke(core.clj:53)
        at jepsen.cli$single_test_cmd$fn__13951.invoke(cli.clj:396)
        at jepsen.cli$run_BANG_.invokeStatic(cli.clj:329)
        at jepsen.cli$run_BANG_.invoke(cli.clj:258)
        at maelstrom.core$_main.invokeStatic(core.clj:269)
        at maelstrom.core$_main.doInvoke(core.clj:267)
        at clojure.lang.RestFn.applyTo(RestFn.java:137)
        at maelstrom.core.main(Unknown Source)

Expected Behavior: The default workload, which is "lin-kv", should be used when the -w option is not explicitly provided. Furthermore, when I run the test with the explicit -w lin-kv argument, the test works. It appears there's an issue with either the default value behavior or the way it's being handled.

We can either fix or remove the lin-kv option from the default settings. I am open to contributing a fix through a pull request based on the direction you would like to take on this issue.

aphyr commented 1 year ago

I think the :default should probably be a keyword, not a string, here.On Sep 10, 2023 09:40, renancloudwalk @.***> wrote: Found a minor issue when running maelstrom against a binary without explicitly setting the workload -w option, the test throws a NullPointerException. Probably the workload set here (def test-opt-spec "Options for single tests." [[nil "--bin FILE" "Path to binary which runs a node" :missing "Expected a --bin PATH_TO_BINARY to test"]

["-w" "--workload NAME" "What workload to run." :default "lin-kv" :parse-fn keyword :validate [workloads (cli/one-of workloads)]] ])

Is not setting the workload-name here workload-name (:workload opts) workload ((workloads workload-name) (assoc opts :nodes nodes, :net net))

Reproduction Steps: Execute the command: ./maelstrom test --bin demobin the error message which includes the main details: ERROR [2023-09-09 21:22:36,041] main - jepsen.cli Oh jeez, I'm sorry, Jepsen broke. Here's why: java.lang.NullPointerException: Cannot invoke "clojure.lang.IFn.invoke(Object)" because the return va at maelstrom.core$maelstrom_test.invokeStatic(core.clj:62) at maelstrom.core$maelstrom_test.invoke(core.clj:53) at jepsen.cli$single_test_cmd$fn__13951.invoke(cli.clj:396) at jepsen.cli$runBANG.invokeStatic(cli.clj:329) at jepsen.cli$runBANG.invoke(cli.clj:258) at maelstrom.core$_main.invokeStatic(core.clj:269) at maelstrom.core$_main.doInvoke(core.clj:267) at clojure.lang.RestFn.applyTo(RestFn.java:137) at maelstrom.core.main(Unknown Source)

Expected Behavior: The default workload, which is "lin-kv", should be used when the -w option is not explicitly provided. Furthermore, when I run the test with the explicit -w lin-kv argument, the test works. It appears there's an issue with either the default value behavior or the way it's being handled. We can either fix or remove the lin-kv option from the default settings. I am open to contributing a fix through a pull request based on the direction you would like to take on this issue.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: @.***>

renancloudwalk commented 1 year ago

@aphyr pretty much it!

The fix is here: #80