Skip to content

Limit task history fields consumed from hraven#1472

Merged
ianoc merged 5 commits intotwitter:developfrom
rubanm:rubanm/hraven_content_length_fix
Jan 14, 2016
Merged

Limit task history fields consumed from hraven#1472
ianoc merged 5 commits intotwitter:developfrom
rubanm:rubanm/hraven_content_length_fix

Conversation

@rubanm
Copy link
Contributor

@rubanm rubanm commented Jan 14, 2016

Some jobs internally have been running into http content limits set by hraven. We really only need a few of the task history fields that are currently being consumed.

This change uses the new response filters added in hraven client to limit the amount of data being read:
https://github.com/twitter/hraven/blob/master/hraven-core/src/main/java/com/twitter/hraven/rest/client/HRavenRestClient.java#L356

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is t05 a public release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ianoc
Copy link
Collaborator

ianoc commented Jan 14, 2016

Could you talk about if we will loose compatibility with older versions of hraven or if its a fall back. Have we manually tested this in real jobs?

(Does anyone outside twitter even use hraven?)

@rubanm
Copy link
Contributor Author

rubanm commented Jan 14, 2016

We will lose compatibility with older versions. The newer version is already deployed internally at twitter. I've run a test cron job to verify hraven history gets picked up correctly. cc @vrushalivc

I'm not aware of anyone outside of twitter using the hraven estimators, so we're probably fine here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

can this be factored out to a method call? The line is super long.

@johnynek
Copy link
Collaborator

This means a version bump if we are strict (not sure we should be for a submodule that maybe no one uses).

@rubanm
Copy link
Contributor Author

rubanm commented Jan 14, 2016

Refactored the long line. Not bumping the version for now (we'll need to go from 0.15 to 0.16 to be strict?).

@ianoc
Copy link
Collaborator

ianoc commented Jan 14, 2016

@johnynek We sort of have been on RC's for too long around 0.16 i guess anyway. But thoughts in general about the backwards incomaptible for external users here. I'm not sure it really matters since I doubt anyone external to twitter uses this. I'd almost be inclined to say in future this is penciled for something that could just go back internal to twitter and not be in the OSS project

@johnynek
Copy link
Collaborator

Yeah, the specific Hraven calls should probably be internal to Twitter. LGTM otherwise.

ianoc added a commit that referenced this pull request Jan 14, 2016
 Limit task history fields consumed from hraven
@ianoc ianoc merged commit bb1c4d2 into twitter:develop Jan 14, 2016
@rubanm rubanm deleted the rubanm/hraven_content_length_fix branch January 14, 2016 19:32
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