Implemented querying concurrent schema - #45
Conversation
shlomi-noach
left a comment
shlomi-noach
left a comment
There was a problem hiding this comment.
Hi, thank you for this PR.
Generally, it would be best to first open an Issue and discuss what it is you're trying to do, and from there on take it to a PR.
As it stands, there's quite some changes I'd appreciate if ccql is to support the change you wish to incorporate.
I'm also concerned with the concurrency model you used for "concurrent hosts" and "concurrent schemas", because the latter is scattered in the former. There's really one concurrency cap and not two. ccql already offers max-concurrency, so why not use that.
Please always be backwards compatible. Your changes will break the behavior for everyone.
| askPassword := flag.Bool("ask-pass", false, "prompt for MySQL password") | ||
| credentialsFile := flag.String("C", "", "Credentials file, expecting [client] scope, with 'user', 'password' fields. Overrides -u and -p") | ||
| defaultSchema := flag.String("d", "information_schema", "Default schema to use") | ||
| schemaList := flag.String("s", "information_schema", "List of Schema to query from.") |
There was a problem hiding this comment.
This is a breaking, incompatible change. Please avoid doing this. Find a backwards compatible way.
| } | ||
| User string | ||
| Password string | ||
| } |
There was a problem hiding this comment.
gofmt will fail on this change of indentation.
| } | ||
|
|
||
| if err := logic.QueryHosts(hosts, *user, *password, *defaultSchema, queries, *maxConcurrency, *timeout); err != nil { | ||
| schemas := strings.Split(*schemaList, ",") |
There was a problem hiding this comment.
please handle whitespace
| } | ||
| rowOutput := strings.Join(output, "\t") | ||
| fmt.Println(rowOutput) | ||
| fmt.Println(defaultSchema, rowOutput) |
| anyError = err | ||
| log.Printf("%s %s", host, err.Error()) | ||
| } | ||
| <-concurrentSchemas |
There was a problem hiding this comment.
the <- should be in a defer function call at the beginning of this routine
|
Thanks for the comments. I apologize as I was not aware of opening an issue before giving out a pull request. I will incorporate these changes and update my PR. Also, should I open an issue now? |
Since we're already here, let's just make progress here. |
| } | ||
| for _, row := range resultData { | ||
| output := []string{host} | ||
| output := []string{host, schema} |
There was a problem hiding this comment.
This is breaking existing behavior: this change forces the addition of the schema column. Perhaps omit the schema column if only a single schema is specified? I'm wondering whether there should be a command line flag to show/hide schema?
There was a problem hiding this comment.
When working with multi sharded environment, the visibility of the schema column becomes an asset. But I agree this can be a breaking change, and I can try to add a command line flag to view the schema and only if that flag is set, the schema column becomes visible.
| anyError = err | ||
| log.Printf("%s %s", host, err.Error()) | ||
| } | ||
| defer wg.Done() |
There was a problem hiding this comment.
defer functions should appear first thing in the enclosing function.
| return err | ||
| } | ||
|
|
||
| fmt.Println() |
There was a problem hiding this comment.
Why is this Println() added?
There was a problem hiding this comment.
When using the flag --ask-pass the results are shown are next to the Enter Password phrase. Will try to make this formatting better.
| output := []string{host} | ||
| if viewSourceSchema { | ||
| outputSchema := []string{schema} | ||
| output = append(output, outputSchema...) |
There was a problem hiding this comment.
simpler:
output = append(output, schema)
| output[0] = host | ||
| output := []string{host} | ||
| if viewSourceSchema { | ||
| outputSchema := []string{schema} |
|
Thank you for this pull request! This is looking good. I may yet tweak with command line params, trying to consider what would make most sense with backward compatibility. Noting that I'll be out on holiday for a while and so merging may be delayed. |
|
That is so good to hear. Looking forward to seeing this merged soon. |
|
@AnshumanTripathi this is now resubmitted as #48 . Please see how I fixed the |
2 participants
Implemented concurrent querying over shards