You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by jacksontj <gi...@git.apache.org> on 2016/04/07 03:59:44 UTC
[GitHub] trafficserver pull request: Ts 3440
GitHub user jacksontj opened a pull request:
https://github.com/apache/trafficserver/pull/554
Ts 3440
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jacksontj/trafficserver TS-3440
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/554.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #554
----
commit 59c3ed195acc2cac719eade3981411ab97cb4b6e
Author: Thomas Jackson <ja...@apache.org>
Date: 2016-04-07T01:58:12Z
Revert "TS-3440: Connect retries shouldn't happen if the connection has succeeeded and data has been sent"
This reverts commit 440a14513e59baf21e55b07f0dd4aa39bac232de.
Conflicts:
proxy/http/HttpTransact.cc
commit 20d605c0f63d80970b4d0ab59f4565f433d89693
Author: Thomas Jackson <ja...@apache.org>
Date: 2016-04-07T01:58:36Z
TS-3440 Not retry if any bytes were sent
Before we would still retry if the origin didn't error-- but just took a really long time. So, instead of only checking if the body was sent-- we can check the milestone to see if we sent the request. If any bytes were sent the request cannot be retried.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4328
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on the pull request:
https://github.com/apache/trafficserver/pull/554#issuecomment-214491833
Can you guys expand on the concern with using milestones for this? I can easily add another bool to the state machine (or change the `request_body_start` to `request_start`) but it seems like a completely duplicated variable-- and I haven't really gotten an explanation of why not except that "its unfortunate"-- which is a bit hard to act on ;).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4328
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the pull request:
https://github.com/apache/trafficserver/pull/554#issuecomment-206675022
I did the original fix for this bug and this missed case definitely makes sense. I'm :+1: with the fix. The only thing that looks a little weird is using milestones as a flag but I suppose that's probably better than adding an unnecessary boolean flag.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4328
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on the pull request:
https://github.com/apache/trafficserver/pull/554#issuecomment-215864574
@zwoop @jpeach @SolidWallOfCode After chatting some more on IRC today, it seems the people are quite against the milestones approach-- so I've changed this patch to check the SM if we wrote header bytes -- which solves my immediate question "did I send bytes". I'm going to file a separate issue for some sort of API to see where the SM has been, since we don't want to use milestones for that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4328
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj closed the pull request at:
https://github.com/apache/trafficserver/pull/554
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4328
Posted by JamesPeach <gi...@git.apache.org>.
Github user JamesPeach commented on the pull request:
https://github.com/apache/trafficserver/pull/554#issuecomment-214490389
As per IRC discussion with @zwoop and @bryancall, I think we should find a way to not use the milestones to track the state transition.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4328
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/554#issuecomment-207223148
I looked at this for a while and I'm not convinced that it doesn't have unindented side-effects. ``is_request_retryable`` is only set once POST data is getting sent. Checking the milestone makes more request types non-retryable.
The use of milestones is fairly unfortunate as well; I'd really like to avoid that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4328
Posted by sudheerv <gi...@git.apache.org>.
Github user sudheerv commented on the pull request:
https://github.com/apache/trafficserver/pull/554#issuecomment-214693606
The problem is that you are trying to "mix" the internals of a completely independent/different feature to use for an unrelated/unintended purposes. This sets a bad precedent and makes the code harder to maintain.
You are looking at one specific example where it seems to be conveniently doing what you need "right now" - but, just bcoz it seems to be satisfying the needs right now doesn't mean it will continue to do so in the future, since milestones is a different feature/function intended to do completely different things. If tomorrow someone changes milestone implementation which is not entirely unlikely, it affects the rest of the code that depends on it so tightly because of its usage as "more" than milestones. Again, you might argue that when changing milestones one could also go ahead and change the rest of the code that "depends" on it. But, you now probably see the issue there? Think of people following the precedent set and start to randomly use milestones all over the place for various unrelated purposes. Milestones feature is no longer doing just milestones :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4328
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on the pull request:
https://github.com/apache/trafficserver/pull/554#issuecomment-207539560
After chatting with @jpeach we agree on the goal here (to not retry if bytes were sent on the wire). I have updated the PR to include comments attempting to explain the situation without the 10m chat we had on IRC ;)
In addition since this effectively deprecates `request_body_start` (no one else uses it) I've gone ahead and removed it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4328
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on the pull request:
https://github.com/apache/trafficserver/pull/554#issuecomment-214621535
Well, I'm not sure that I agree it violates the encapsulation-- since its effectively a variable that tracks timings (which are only set in specific states in the state machine-- IIRC most are only set once) which makes it more or less a history of where the HTTPSM has gone. For this particular feature I need to know "have we sent bytes" and today the only way that is stored is in the milestones. It sounds like both of you guys ( @zwoop and @jpeach ) want me to add a bool to the state machine to store this-- which I can do, but since the milestone is only set in one place, it'll effectively be the same check-- so i still don't really get how that would be "cleaner" in any way :/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4328
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/554#issuecomment-214505724
It's unfortunate because it couple internal state in the SM to the milestones, which are timing events. This means, changes to the milestones could have bad effects on other code, which would be really tricky to debug. It's sort of a violation of encapsulation, or even depending on side effects as a feature.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4328
Posted by sudheerv <gi...@git.apache.org>.
Github user sudheerv commented on the pull request:
https://github.com/apache/trafficserver/pull/554#issuecomment-214694979
Oh, in any case, I haven't really checked carefully, but I actually wonder if the milestone (server begin write) in question here is reset for "each" server leg (assuming there's a 3xx redirect follow or some sort of other mechanism that is triggered)? If (not) so, does that still work the way you intended?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---