OPER-2071 Port Tapjoy changes to latest New Relic gem

[alanbrent]

This is required work for OPER-2045, related PR here

@Tapjoy/eng-group-ops

[alanbrent]

Forward-ported commit in question is here

[atayarani]

ping @jlogsdon since you were involved in the discussion earlier today

[alanbrent]

@jlogsdon @obrie if you could look at this, that'd be great. If you checkout my branch and run git diff 0dfcd603acd4277a1d9da19722265a57b1499e93 lib/new_relic/agent/pipe_channel_manager.rb that should give you the difference.

[StabbyCutyou]

This seems like a lot of changes to review.

Wondering if it'd be easier to take the current gem and try to replay our change on top of it?

[StabbyCutyou]

Actually, looks like a pretty clean merge

[alanbrent]

Mostly, yeah. I did have to resolve a conflict during rebase, but it was
reasonably straightforward.

On Wednesday, March 2, 2016, StabbyCutyou notifications@github.com wrote:

Actually, looks like a pretty clean merge

—
Reply to this email directly or view it on GitHub
#3 (comment).

Brent Stephens

[obrie]

lgtm! Thanks for doing this!

[StabbyCutyou]

Should we do a burn-in test on chore for an hour or so, to see if the
problem recurs? I remember how to detect it.

But this looked good in general
On Mar 3, 2016 06:51, "Aaron Pfeifer" notifications@github.com wrote:

lgtm! Thanks for doing this!

—
Reply to this email directly or view it on GitHub
#3 (comment).

[obrie]

You could use chore-tester -- I think it typically only took a few minutes to detect it using that (we can use the aws stub to avoid having to load anything into sqs).

[obrie]

It's probably worth it to at least do the chore-tester test before rolling this out to TJS.

[StabbyCutyou]

I was going to ping Lionel, Alvin, or Jayanth since I think they've been working on chore-tester, but they don't have access to this repo so I can't do that.

I'll drop them an email to see if they have time to help test.

[StabbyCutyou]

Jayanth is going to do some testing for us today.

[alanbrent]

Should I change the build string to something a little more indicative of custom-ness? e.g. 3.15.0.314.TJ

[goose42]

@StabbyCutyou @obrie @alanbrent

Ran some tests using chore-tester. Had this configuration:

c.consumer_strategy   = Chore::Strategy::ThreadedConsumerStrategy
c.worker_strategy     = Chore::Strategy::ForkedWorkerStrategy
c.batch_size          = 1
c.threads_per_queue   = 1
c.num_workers         = 10

Also, this was running on an Ubuntu machine, it read through 10000 messages, and the the number of open files were consistently between 62 and 64 during the entire execution.

[alanbrent]

GIMME THE THUMB

[alanbrent]

Would it be better for me to bring upstream into master with a separate PR? That would leave this PR with a single commit.

[alanbrent]

#4

[obrie]

My general strategy in the past has been to:

1. Keep master up-to-date with the upstream
2. Create a tj-{version} branch
3. PR into that branch
4. (I think) release with a teeny version bump

[obrie]

(updated comment since I hit submit a little too quickly)

[StabbyCutyou]

We add a 4th level to semver, i think, for managing our own version of someone elses gem.

So if this were 1.2.3, our version would be 1.2.3.1

[alanbrent]

Last time we just incremented the build number by 1, which is what I've done
here so far. Want to go with .1 instead?

On Friday, March 4, 2016, StabbyCutyou notifications@github.com wrote:

We add a 4th level to semver, i think, for managing our own version of
someone elses gem.

So if this were 1.2.3, our version would be 1.2.3.1

—
Reply to this email directly or view it on GitHub
#3 (comment).

Brent Stephens

[StabbyCutyou]

The problem is that if the existing gem is 1.2.3, and we bump our copy to 1.2.4, it now sort of conflicts with the "Real" gem if that one ever bumps to 1.2.4.

I recommend we tack on our own 4th semver particle, since it's A: compatible with the way bundler works and B: has no actual downside to it.

[alanbrent]

Of course you reply after I've just finished building a TJS slug! And yeah,
I completely agree with you and will change the build number. Which doesn't
require a change in this repo.

Brent Stephens

On Fri, Mar 4, 2016 at 9:33 AM, StabbyCutyou notifications@github.com
wrote:

The problem is that if the existing gem is 1.2.3, and we bump our copy to
1.2.4, it now sort of conflicts with the "Real" gem if that one ever bumps
to 1.2.4.

I recommend we tack on our own 4th semver particle, since it's A:
compatible with the way bundler works and B: has no actual downside to it.

—
Reply to this email directly or view it on GitHub
#3 (comment).

[alanbrent]

Okay, so ... should we open a pull request upstream to obviate the need for customizing this gem? If so, I'm sure I can't be the one answering their inevitable questions :)

[obrie]

Yeah I think we should be opening a PR, though I suspect that the change we've made is a workaround rather than a root cause. It'll take a little time to gather all of the information and examples needed to have a productive conversation -- but it's something we should definitely do.

[jlogsdon]

I am pretty sure we've tried pushing this upstream before and it wasn't accepted.

[alanbrent]

FYI, the only upstream PR I found by us through a quick search was newrelic#135, which was a previous-to-this changeset and was accepted (yay!). Oddly enough, this patch we're forward-porting was committed the same day as that PR was opened. I imagine the intent was to go ahead and PR this one later, and it dropped out of memory.

So ... seems best to leave this ball in @obrie's court.

[obrie]

That was my understanding (according to my admittedly terrible memory) :)

I'm happy to take on the responsibility of figuring out how to get this upstream.

[StabbyCutyou]

Blackmail would be my first suggestion

On Mon, Mar 7, 2016 at 2:58 PM, Aaron Pfeifer notifications@github.com
wrote:

That was my understanding (according to my admittedly terrible memory) :)

I'm happy to take on the responsibility of figuring out how to get this
upstream.

—
Reply to this email directly or view it on GitHub
#3 (comment).

[alanbrent]

FYI the activity on this PR was me updating to a recently released update, finding out it breaks TJS specs, and reverting. We're back to the previous state, and this PR is ready for thumbs.