Skip to content

RFR: statsd ingestion endpoint.#201

Merged
gdusbabek merged 27 commits intorax-maas:masterfrom
gdusbabek:http_bulk_ingest_aggregates
Jan 23, 2014
Merged

RFR: statsd ingestion endpoint.#201
gdusbabek merged 27 commits intorax-maas:masterfrom
gdusbabek:http_bulk_ingest_aggregates

Conversation

@gdusbabek
Copy link
Copy Markdown
Contributor

A few things to know up front:

  • Ingestion is a blocking HTTP operation (I'm amenable to changing this, but wanted feedback first).
  • I punted on TTL, using a 48H default TTL for now.

Sample ingestion format is found in blueflood-http/src/test/resources/sample_bundle.json.

This PR is closely related to the statsd-backend that currently lives at https://github.com/gdusbabek/blueflood-statsd-backend.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure why this is var args?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's the same signature as Arrays.asList().

@lakshmi-kannan
Copy link
Copy Markdown
Contributor

I like this PR. Seems like you are on the right track.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be nice if all the boiler plate to parse the payload goes into their own file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved in 56186d4.

@lakshmi-kannan
Copy link
Copy Markdown
Contributor

Mostly LGTM. (Pending my comments & nits).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What happens if IOError gets bubbled up? Are we catching it somewhere?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it will get caught in the threadpool now and result in a 500.

…ly, as some AstyanaxWriter paths cannot handle string/boolean metrics.

Share processors among HTTP handlers.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doesn't this mean we share the thread pool for statsd and regular metrics? I am fine with this approach. Just wanted to confirm that this is intentional.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was done intentionally.

@lakshmi-kannan
Copy link
Copy Markdown
Contributor

+1 (pending fixing the nits)

gdusbabek added a commit that referenced this pull request Jan 23, 2014
@gdusbabek gdusbabek merged commit 6e60b69 into rax-maas:master Jan 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants