mt-store-cat improvements: glob filter, chunk-csv output #1147
Conversation
use functions that start with 'print' as the main printing workers and better comments
Dieterbe
changed the title
Dieterbe
force-pushed
the
mt-store-cat-improvements
branch
from
2cd8b33 to
9cc3a5f
Compare
| @@ -266,22 +293,28 @@ func main() { | |||
| fmt.Printf("metric id %v not found", amkey.MKey) | |||
| return | |||
| } | |||
| for _, m := range metrics { | |||
| fmt.Println(m) | |||
| if verbose { | |||
There was a problem hiding this comment.
i'd just create some wrapper function logIfVerbose(). I guess we don't need to worry about an extra function call here
| @@ -194,24 +196,31 @@ func main() { | |||
| log.Fatalf("failed to read tables from cassandra. %s", err.Error()) | |||
| } | |||
|
|
|||
| // set up is done, now actually execute the business logic | |||
|
|
|||
| // handle "tables" | |||
| if tableSelector == "tables" { | |||
There was a problem hiding this comment.
we're assuming that there will never be a table called tables i guess
There was a problem hiding this comment.
yes. which seems fair
| if strings.HasPrefix(metricSelector, "substr:") { | ||
| substr = strings.Replace(metricSelector, "substr:", "", 1) | ||
| } | ||
| if strings.HasPrefix(metricSelector, "glob:") { |
There was a problem hiding this comment.
This section with so many redundant strings.HasPrefix() seems kind of strange to me. Wouldn't it be way cleared if there's simply one case for each of the possible metric selector methods. Then each of those metric selectors could have its own matching method instead of the current match(prefix, substr, glob, ...) which needs to go through all the possible matching methods even though only one of them can ever be used at once. That selector-specific matching method could then simply be passed into getMetrics() by reference. This might be personal taste, but i think that would be cleaner.
There was a problem hiding this comment.
even though only one of them can ever be used at once
in the future i'ld like to combine multiple selectors. e.g. prefix and substr.
| func printChunkCsv(ctx context.Context, store *cassandra.CassandraStore, table cassandra.Table, metrics []Metric, start, end uint32) { | ||
|
|
||
| // see CassandraStore.SearchTable for more information | ||
| start_month := start - (start % cassandra.Month_sec) // starting row has to be at, or before, requested start |
There was a problem hiding this comment.
Possible I just don't get it why this is necessary... but:
start_month is only used 2 lines down, where it will be divided by cassandra.Month_sec. Since integer divisions would drop the start % cassandra.Month_sec anyway, how is this code different from:
startMonthNum := start / cassandra.Month_sec
There was a problem hiding this comment.
that sounds good.
i suppose similarly we can also do:
endMonthNum := (end - 1) / cassandra.Month_sec
right?
note: this code comes from the cassandra code, and i think we do it elsewhere too. so perhaps better to do a separate cleanup PR for just that.
There was a problem hiding this comment.
yeah i think you're right. i wasn't completely sure by using logic, so i just tested it: https://play.golang.org/p/DOl02-EibSU
Dieterbe
force-pushed
the
mt-store-cat-improvements
branch
from
19b9383 to
d174fbe
Compare
No description provided.