restore SetMaxInFlight dropped in refactoring

[jehiah]

Consumer needs to export a way to update the max-in-flight as a way to pause or resume a client from doing work. This was accidentally dropped in #30

[jehiah]

The importance of this over updating config directly is that it also sends out the updated value to all open connections

[jehiah]

RFR @mreiferson

[mreiferson]

For the record, I dropped this function intentionally, but forgot to restore the related functionality. Oops.

My goal was to eliminate any ambiguity for The Right Way™ to set config parameters. Unfortunately, this change goes back in the other direction as implemented.

I have two suggestions for alternative approaches:

1. The Config struct can publish changes over a callback/delegate/channel (whatever) and interested parties (like Consumer) can listen and act as necessary. This keeps the api consistent and provides a way to restore the behavior.
2. Maybe all we actually want is a way to {Un,}Pause() a Consumer? Should we just provide that API?

Thoughts?

[jehiah]

This really brings to mind that perhaps the whole config struct should be immutable once passed to a Consumer or Producer object, and that things that need to be mutated (like this) need their own Interface. It's possible some could be changed and have a delegate system to mutate existing state, but many can not (like changing tls settings).

I'm also sensitive to avoiding changes in the api where we don't really need them because it's busy work multiplied by the size of code base using the existing api (which for me is a real pain point).

To me, changing the max-in-flight is slightly diff need than pause/unpause, though that's certainly one use.

[mreiferson]

I think making Config immutable after being passed in makes sense. For what it's worth, you could just whitelist a set of vars that were OK to mutate (and then implement the delegate for listening to changes, etc.).

RE: changes in general - this is our "one" opportunity to clean things up while we are already making backwards incompatible changes. It seemed inconsistent to set max_in_flight different from the other properties. I'm not insensitive to the fact that it's a breaking change, it just doesn't really matter once the thing already won't compile due to other changes.

I'd like to think I've written a fair number of consumers and haven't once used the functionality we're discussing. Obviously that doesn't mean there aren't use cases or that folks weren't depending on being able to do this (somehow). But it seems worth discussing, what actually is your use case?

[jehiah]

The use case is generally to be able to dynamically change throughput based on server state. We currently trigger this by touching or removing a file, and use fsnotify to observe that state change and make appropriate changes in max in flight. (obviously all our consumers also use SetMaxInFlight to set their initial throughput settings)

[mreiferson]

Right, but that's pausing and unpausing. I should have been more specific, I was asking the question about arbitrarily adjusting max_in_flight at runtime beyond the pause/unpause case.

[jehiah]

@mreiferson I'll let you propose some other ways to change this API wise before we hit 1.0, but i'm going to merge to restore this for now so i'm no longer blocked on this functionality in incorporating these api changes into the bitly stack.

[mreiferson]

I've proposed some alternatives here. I'm curious to hear your thoughts on those...

You need not be blocked on moving forward with your changes just because this wasn't merged. There are always ways of building off of branches in the interim.

Let's reach agreement before we merge things where there isn't obvious resolution.

[mreiferson]

One more suggestion.

Perhaps it's really just the name that bothers me. I would like the name reflect that it isn't necessary to use this method to set/configure the max_in_flight initially but rather that its purpose is to provide a way to adjust it while a Consumer is running.

What about AdjustMaxInFlight or ResetMaxInFlight? Does that communicate its intended use case better?

I would also like the function comments to reflect its intended use case more accurately (for example, it still refers to "reader").

[jehiah]

or NewMaxInFlight or ChangeMaxInFlight? I see where you are going w/ naming, but i'm not sure what's ideal. Also comments in #47 imply a change that would enforce the config object can't be used to change once a Consumer is spun up.

[mreiferson]

New doesn't work because of the go conventions for that to instantiate something. I think Change or Adjust are basically equal and I'm fine with either. Along with comment documentation improvements, these changes would make this a lot clearer.

Would you be amenable to making these changes?

[jehiah]

naming was modified to ChangeMaxInFlight() in #52