search

openark / orchestrator

MySQL replication topology management and HA
Apache License 2.0
5.63k stars 930 forks source link

orchestrator-client error on submit-masters-to-kv-stores #533

Open gtowey opened 6 years ago

gtowey commented 6 years ago

Getting the error:

$ orchestrator-client  -c submit-masters-to-kv-stores -alias workflow-orchestration
jq: error (at <stdin>:1): Cannot iterate over null (null)

This is because the script doesn't handle all the possible outputs of the API call:

$ orchestrator-client -c api -path  submit-masters-to-kv-stores/workflow-orchestration 
{ "Code": "OK", "Message": "Submitted 0 masters", "Details": null }
gtowey commented 6 years ago

@shlomi-noach I can submit a fix for this one, but I would like to get your thoughts on the best way to approach it. I can think of two ways:

  1. Return an error from the API when there are zero masters
  2. change orchestrator-client to handle the null result in Details field

In this case I think #2 is easier, but #1 is the correct way. When using this command I would really like to know why there are zero masters submitted.

shlomi-noach commented 6 years ago

There are two valid ways to invoke submit-masters-to-kv-stores/: with our without a cluster alias. When without alias, orchestrator will try and submit everything. When with cluster alias, it will only submit KV for that cluster.

I think it's clear that if we submit a single cluster, there must be an error returned when no master is found. The API should return that error; orchestrator-client should remain relatively stupid.

In the event of multiple clusters, though... I don't see an easy way out. I'd keep the behavior as it is.

Incidentally I took a look yesterday and the fact we got a Details: null is actually a golang "optimization" (I expected Details to be an empty array). I've seen this multiple times in the past.

gtowey commented 6 years ago

@shlomi-noach maybe we can add a concept of warning output to the response message? So that when you're adding all clusters, those with no valid masters can be flagged as warnings instead of errors and the command can be allowed to run successfully.

shlomi-noach commented 6 years ago

Related: https://github.com/github/orchestrator/pull/549, auto-populating KV store.

shlomi-noach commented 6 years ago

Related: https://github.com/github/orchestrator/pull/548, a read-only master is valid for submit-masters-to-kv-stores.