Bigtable idx and store plugins #1082
Conversation
| @@ -244,14 +244,14 @@ func TestCorruptionCase2(t *testing.T) { | |||
| switch action { | |||
| case 0: | |||
| chunk := getRandomNumber(0, 100) | |||
| t.Logf("adding chunk %d", chunk) | |||
| //t.Logf("adding chunk %d", chunk) | |||
There was a problem hiding this comment.
those should probably be deleted before merging
cmd/metrictank/metrictank.go
Outdated
| if bigtableStore.CliConfig.Enabled { | ||
| store, err = bigtableStore.NewStore(bigtableStore.CliConfig, mdata.TTLs()) | ||
| if err != nil { | ||
| log.Fatalf("failed to initialize cassandra. %s", err) |
There was a problem hiding this comment.
I think that should be bigtable instead of cassandra
| MT_CASSANDRA_IDX_ENABLED: "false" | ||
| MT_BIGTABLE_STORE_ENABLED: "true" | ||
| MT_BIGTABLE_IDX_ENABLED: "true" | ||
| BIGTABLE_EMULATOR_HOST: bigtable:8086 |
There was a problem hiding this comment.
how do you feel about this variable that bypasses MT's config system? wouldn't it be cleaner to make this an MT config param? i notice that for "real" (non-emulated) bigtable, there is no addr setting though.
There was a problem hiding this comment.
This is not processed by MT. This is directly handled by the bigtable client library. In the same way GOGC is handled directly by the runtime.
Given that this is purely for development, i see no reason to add support for it in the config
|
@woodsaj could you write down a short overview of the design of the plugins. maybe some bullet points about how it compares to the cassandra plugins for example, could save me time . |
idx/bigtable/schema.go
Outdated
| case "Id": | ||
| mkey, err := schema.MKeyFromString(string(col.Value)) | ||
| if err != nil { | ||
| log.Errorf("bigtable-idx: load() could not parse ID %q: %s -> skipping", string(col.Value), err) |
There was a problem hiding this comment.
why does this function have 3 different ways to handle errors?
- some errors returned to caller, at which point caller logs
"bigtable-idx: failed to marshal row to metricDefas well as the returned error - on this line, we log the error directly and skip reading the column, but continue reading other columns and return err=nil to the caller, this looks like a bug
- at the bottom of the loop, we Fatalf if we encounter an unknown column because "if we get bad data doesnt seem unreasonable"
seems we should treat all cases the same. i suggest to immediately return the error in all cases, and log.Fatalf in the caller
woodsaj
force-pushed
the
bigtable
branch
2 times, most recently
from
58fd77e
to
68ac948
Compare
|
@woodsaj thanks for the doc. that is more than i bargained for and much appreciated. |
The writeQueue will be closed when the plugin is shutdown. Before exiting we should flush any pending writes that are in the buffer.
- use a config struct for both store and idx plugins - store commandLine args in CliConfig package global - pass CliConfig to the plugin constructor - validate config as early as possible
100000 is the hard coded limit of mutations in a single bigtable write request
Both the api and bigtable store need a Limiter to constrain concurrency. This is an updated version of the limiter that was used in api/dataprocessor. It has been updated to take a context so that calls to Acquire() can be canceled.
|
ok @Dieterbe this should be good to go now. |
|
@woodsaj i added a few commits based on some feedback that was not addressed and also found some typos etc. please check the 3 commits i pushed. if they look good to you, please merge this. |
|
Thanks @Dieterbe. I don't like the approach used for validating the max-chunk-span, I think that the Validate() method should not have any arguments. But what you have works, so lets just keep it for now. Providing consistent config handling and understanding dependencies between components will be addressed #1031 |
1 similar comment
|
Thanks @Dieterbe. I don't like the approach used for validating the max-chunk-span, I think that the Validate() method should not have any arguments. But what you have works, so lets just keep it for now. Providing consistent config handling and understanding dependencies between components will be addressed #1031 |
|
Thanks @Dieterbe. I don't like the approach used for validating the max-chunk-span, but what you have works, so lets just keep it for now. As part of #1031 this will all get refactored. Until we have designed the config/plugin model we want to adapt within the code base, there is no point in doing anything more than simply addressing the current functional requirements. |
No description provided.