Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

New flag 'ingest-from'#1382

Merged
fkaleo merged 22 commits intomasterfrom
ingest_after
Aug 12, 2019
Merged

New flag 'ingest-from'#1382
fkaleo merged 22 commits intomasterfrom
ingest_after

Conversation

@fkaleo
Copy link
Copy Markdown
Contributor

@fkaleo fkaleo commented Jul 5, 2019 •
edited
Loading

New flag 'ingest-from' that instructs metrictank to discard any point of a specific org id that does not belong to chunks starting after a given timestamp.
e.g. 20:67031 with a chunk span of 10 will discard all points with org id 20 and timestamps lesser than 67040

@woodsaj
Copy link
Copy Markdown
Contributor

woodsaj commented Jul 5, 2019

why limit this to only 1 orgId? Whould it not be better to just have AggMetrics use a map of orgId's to ingestAfter timstamp. eg ingestAfter map[int32]int32

Users can then pass a list of org:ts pairs via the config. eg
metrictank --ingest-after=1:1562339919,1234:1562339919,45:1562339919

@fkaleo
Copy link
Copy Markdown
Contributor Author

fkaleo commented Jul 8, 2019

why limit this to only 1 orgId? Whould it not be better to just have AggMetrics use a map of orgId's to ingestAfter timstamp. eg ingestAfter map[int32]int32

Users can then pass a list of org:ts pairs via the config. eg
metrictank --ingest-after=1:1562339919,1234:1562339919,45:1562339919

The only reason was the fact that we did not need it and kept the code a little simpler. We can always add it later.

@fkaleo
Copy link
Copy Markdown
Contributor Author

fkaleo commented Jul 8, 2019

why limit this to only 1 orgId? Whould it not be better to just have AggMetrics use a map of orgId's to ingestAfter timstamp. eg ingestAfter map[int32]int32
Users can then pass a list of org:ts pairs via the config. eg
metrictank --ingest-after=1:1562339919,1234:1562339919,45:1562339919

The only reason was the fact that we did not need it and kept the code a little simpler. We can always add it later.

Using a map is probably less code though; let me try that.

@fkaleo fkaleo changed the title [WIP] New flag 'ingest-after' New flag 'ingest-after' Jul 16, 2019
@fkaleo fkaleo marked this pull request as ready for review July 16, 2019 08:50
@fkaleo fkaleo requested a review from robert-milan July 30, 2019 15:59
robert-milan
robert-milan reviewed Jul 31, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@robert-milan robert-milan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might need to be rebased on master

Comment thread mdata/aggmetric.go Outdated
@fkaleo
Copy link
Copy Markdown
Contributor Author

fkaleo commented Jul 31, 2019

I think this might need to be rebased on master

Done

Dieterbe
Dieterbe reviewed Jul 31, 2019
View reviewed changes
Comment thread cmd/metrictank/metrictank.go Outdated
Dieterbe
Dieterbe reviewed Aug 7, 2019
View reviewed changes
Comment thread mdata/aggmetric.go Outdated
Dieterbe
Dieterbe reviewed Aug 8, 2019
View reviewed changes
Comment thread util/flag_parsers_test.go Outdated
@fkaleo fkaleo force-pushed the ingest_after branch 3 times, most recently from 6a00142 to 3bb1d5f Compare August 8, 2019 13:01
Dieterbe
Dieterbe reviewed Aug 8, 2019
View reviewed changes
Comment thread mdata/aggmetric.go Outdated
@fkaleo fkaleo changed the title New flag 'ingest-after' New flag 'ingest-from' Aug 8, 2019
Dieterbe
Dieterbe reviewed Aug 8, 2019
View reviewed changes
Comment thread mdata/aggregator_test.go
expected = []schema.Point{
{Val: 5, Ts: 120},
}
compare("simple-min-ingest-from-on-chunk-boundary", agg.minMetric, expected)
Copy link
Copy Markdown
Contributor

@Dieterbe Dieterbe Aug 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too tired now, but tomorrow i'ld like to look into this one last thing, about the timestamps going 130->120 and how that may or should not affect things.

Copy link
Copy Markdown
Contributor Author

@fkaleo fkaleo Aug 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. just need to look at one last thing tomorrow.

Thank you! Your changes look great.

@Dieterbe
Copy link
Copy Markdown
Contributor

Dieterbe commented Aug 8, 2019

LGTM. just need to look at one last thing tomorrow.

Dieterbe
Dieterbe reviewed Aug 9, 2019
View reviewed changes
Comment thread mdata/aggmetric_test.go
Dieterbe
Dieterbe reviewed Aug 9, 2019
View reviewed changes
Comment thread mdata/aggregator_test.go
Dieterbe
Dieterbe approved these changes Aug 9, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@Dieterbe Dieterbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Please have a close look at the last 2 commits I added.
If they make sense, merge this.
If not, let's talk.

@fkaleo
Copy link
Copy Markdown
Contributor Author

fkaleo commented Aug 12, 2019 •
edited
Loading

LGTM
Please have a close look at the last 2 commits I added.
If they make sense, merge this.
If not, let's talk.

They make sense. I'm adding test cases to cover the last one.

@Dieterbe
Copy link
Copy Markdown
Contributor

Dieterbe commented Aug 12, 2019

I already covered that in the test case i added.
if we need any more tests, it should be one that uses an actual AggMetric with aggregators on it, and confirms the right points make it in to the aggregator (points in the last aggspan before the first t0)

@fkaleo
Copy link
Copy Markdown
Contributor Author

fkaleo commented Aug 12, 2019

I already covered that in the test case i added.
if we need any more tests, it should be one that uses an actual AggMetric with aggregators on it, and confirms the right points make it in to the aggregator (points in the last aggspan before the first t0)

The test case added does not fail when removing the code changes.
And yep, I'm adding a test to an AggMetric and discovering that in fact more code changes need to happen to make it pass, I think.

@fkaleo
Copy link
Copy Markdown
Contributor Author

fkaleo commented Aug 12, 2019

Please take a look at the 2 additional tests and the change in code and let me know what you think.

Dieterbe
Dieterbe reviewed Aug 12, 2019
View reviewed changes
Comment thread mdata/aggmetric_test.go Outdated
@fkaleo fkaleo merged commit e2c64e2 into master Aug 12, 2019
@fkaleo fkaleo deleted the ingest_after branch August 12, 2019 14:04
@Dieterbe Dieterbe mentioned this pull request Aug 12, 2019
Merged
@fkaleo fkaleo mentioned this pull request Oct 12, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@Dieterbe Dieterbe Dieterbe approved these changes

@shanson7 shanson7 shanson7 left review comments

@robert-milan robert-milan robert-milan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@fkaleo @woodsaj @Dieterbe @shanson7 @robert-milan