You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by Leif Hedstrom <zw...@apache.org> on 2018/08/06 21:14:30 UTC
[PROPOSAL] Rename TSHttpTxnSetHttpRetStatus()
We have this API:
void TSHttpTxnSetHttpRetStatus(TSHttpTxn txnp, TSHttpStatus http_retstatus);
Which on its own is a little strange, but looking at code, and talking to Alan, I believe this was added when we added remap plugins long ago, to allow such plugins to modify status codes without having an HttpHdr response.
So, I guess that is fine, and we use it fairly frequently, but looking at this, the naming is really non-standard. I’d like to propose that we change this API to
void TSHttpTxnStatusSet(TSHttpTxn txnp, TSHttpStatus status);
This seems much more inline with the other StatusSet that we have, TSHttpHdrStatusSet().
If this is reasonable, I’d also like to add a TSHttpTxnStatusGet(TSHttpTxn txnp) call.
In addition, I’m contemplating adding two new APIs, on the same note:
void TSHttpTxnStatusSet(TSHttpTxn txnp, const char* reason);
const char* TSHttpTxnStatusGet(TSHttpTxn txnp);
Thoughts? Since this is an API change, I’d like to do the API rename change for v8.0.0, but I can defer the last two API additions to 8.1.0 or 9.0.0 whichever comes first (and peoples preferences).
Cheers,
— leif
Re: [PROPOSAL] Rename TSHttpTxnSetHttpRetStatus()
Posted by Bryan Call <bc...@apache.org>.
+1
-Bryan
> On Aug 6, 2018, at 2:14 PM, Leif Hedstrom <zw...@apache.org> wrote:
>
> We have this API:
>
> void TSHttpTxnSetHttpRetStatus(TSHttpTxn txnp, TSHttpStatus http_retstatus);
>
>
> Which on its own is a little strange, but looking at code, and talking to Alan, I believe this was added when we added remap plugins long ago, to allow such plugins to modify status codes without having an HttpHdr response.
>
> So, I guess that is fine, and we use it fairly frequently, but looking at this, the naming is really non-standard. I’d like to propose that we change this API to
>
> void TSHttpTxnStatusSet(TSHttpTxn txnp, TSHttpStatus status);
>
>
>
> This seems much more inline with the other StatusSet that we have, TSHttpHdrStatusSet().
>
>
> If this is reasonable, I’d also like to add a TSHttpTxnStatusGet(TSHttpTxn txnp) call.
>
>
> In addition, I’m contemplating adding two new APIs, on the same note:
>
> void TSHttpTxnStatusSet(TSHttpTxn txnp, const char* reason);
> const char* TSHttpTxnStatusGet(TSHttpTxn txnp);
>
>
> Thoughts? Since this is an API change, I’d like to do the API rename change for v8.0.0, but I can defer the last two API additions to 8.1.0 or 9.0.0 whichever comes first (and peoples preferences).
>
> Cheers,
>
> — leif
>
Re: [PROPOSAL] Rename TSHttpTxnSetHttpRetStatus()
Posted by Chris Lemmons <al...@gmail.com>.
+1 on both points.
On Mon, Aug 6, 2018 at 3:53 PM Derek Dagit <de...@oath.com.invalid> wrote:
>
> +1 for the rename
> +1 for adding the two new functions
>
> On Mon, Aug 6, 2018 at 4:41 PM, Alan Carroll <
> solidwallofcode@oath.com.invalid> wrote:
>
> > +1
> >
> > On Mon, Aug 6, 2018 at 4:18 PM Leif Hedstrom <zw...@apache.org> wrote:
> >
> > >
> > >
> > > > On Aug 6, 2018, at 3:14 PM, Leif Hedstrom <zw...@apache.org> wrote:
> > > >
> > > > We have this API:
> > > >
> > > > void TSHttpTxnSetHttpRetStatus(TSHttpTxn txnp, TSHttpStatus
> > > http_retstatus);
> > > >
> > > >
> > > > Which on its own is a little strange, but looking at code, and talking
> > > to Alan, I believe this was added when we added remap plugins long ago,
> > to
> > > allow such plugins to modify status codes without having an HttpHdr
> > > response.
> > > >
> > > > So, I guess that is fine, and we use it fairly frequently, but looking
> > > at this, the naming is really non-standard. I’d like to propose that we
> > > change this API to
> > > >
> > > > void TSHttpTxnStatusSet(TSHttpTxn txnp, TSHttpStatus status);
> > > >
> > > >
> > > >
> > > > This seems much more inline with the other StatusSet that we have,
> > > TSHttpHdrStatusSet().
> > > >
> > > >
> > > > If this is reasonable, I’d also like to add a
> > > TSHttpTxnStatusGet(TSHttpTxn txnp) call.
> > > >
> > > >
> > > > In addition, I’m contemplating adding two new APIs, on the same note:
> > > >
> > > > void TSHttpTxnStatusSet(TSHttpTxn txnp, const char* reason);
> > > > const char* TSHttpTxnStatusGet(TSHttpTxn txnp);
> > >
> > >
> > >
> > > Sigh, the last two should be
> > >
> > > void TSHttpTxnReasonSet(TSHttpTxn txnp, const char* reason);
> > > const char* TSHttpTxnReasonGet(TSHttpTxn txnp);
> > >
> > >
> > > The point being, I might want to set the transaction return code to a 403
> > > in a remap plugin, but there’s no way to set a custom reason phrase right
> > > now.
> > >
> > > Cheers,
> > >
> > > — leif
> > >
> > >
> > >
> >
> > --
> > *Beware the fisherman who's casting out his line in to a dried up
> > riverbed.*
> > *Oh don't try to tell him 'cause he won't believe. Throw some bread to the
> > ducks instead.*
> > *It's easier that way. *- Genesis : Duke : VI 25-28
> >
>
>
>
> --
> Derek
Re: [PROPOSAL] Rename TSHttpTxnSetHttpRetStatus()
Posted by "Steven R. Feltner" <sf...@godaddy.com>.
+1 for the rename of the existing call and the addition of two new APIs
On 8/6/18, 2:53 PM, "Derek Dagit" <de...@oath.com.INVALID> wrote:
+1 for the rename
+1 for adding the two new functions
On Mon, Aug 6, 2018 at 4:41 PM, Alan Carroll <
solidwallofcode@oath.com.invalid> wrote:
> +1
>
> On Mon, Aug 6, 2018 at 4:18 PM Leif Hedstrom <zw...@apache.org> wrote:
>
> >
> >
> > > On Aug 6, 2018, at 3:14 PM, Leif Hedstrom <zw...@apache.org> wrote:
> > >
> > > We have this API:
> > >
> > > void TSHttpTxnSetHttpRetStatus(TSHttpTxn txnp, TSHttpStatus
> > http_retstatus);
> > >
> > >
> > > Which on its own is a little strange, but looking at code, and talking
> > to Alan, I believe this was added when we added remap plugins long ago,
> to
> > allow such plugins to modify status codes without having an HttpHdr
> > response.
> > >
> > > So, I guess that is fine, and we use it fairly frequently, but looking
> > at this, the naming is really non-standard. I’d like to propose that we
> > change this API to
> > >
> > > void TSHttpTxnStatusSet(TSHttpTxn txnp, TSHttpStatus status);
> > >
> > >
> > >
> > > This seems much more inline with the other StatusSet that we have,
> > TSHttpHdrStatusSet().
> > >
> > >
> > > If this is reasonable, I’d also like to add a
> > TSHttpTxnStatusGet(TSHttpTxn txnp) call.
> > >
> > >
> > > In addition, I’m contemplating adding two new APIs, on the same note:
> > >
> > > void TSHttpTxnStatusSet(TSHttpTxn txnp, const char* reason);
> > > const char* TSHttpTxnStatusGet(TSHttpTxn txnp);
> >
> >
> >
> > Sigh, the last two should be
> >
> > void TSHttpTxnReasonSet(TSHttpTxn txnp, const char* reason);
> > const char* TSHttpTxnReasonGet(TSHttpTxn txnp);
> >
> >
> > The point being, I might want to set the transaction return code to a 403
> > in a remap plugin, but there’s no way to set a custom reason phrase right
> > now.
> >
> > Cheers,
> >
> > — leif
> >
> >
> >
>
> --
> *Beware the fisherman who's casting out his line in to a dried up
> riverbed.*
> *Oh don't try to tell him 'cause he won't believe. Throw some bread to the
> ducks instead.*
> *It's easier that way. *- Genesis : Duke : VI 25-28
>
--
Derek
Re: [PROPOSAL] Rename TSHttpTxnSetHttpRetStatus()
Posted by Derek Dagit <de...@oath.com.INVALID>.
+1 for the rename
+1 for adding the two new functions
On Mon, Aug 6, 2018 at 4:41 PM, Alan Carroll <
solidwallofcode@oath.com.invalid> wrote:
> +1
>
> On Mon, Aug 6, 2018 at 4:18 PM Leif Hedstrom <zw...@apache.org> wrote:
>
> >
> >
> > > On Aug 6, 2018, at 3:14 PM, Leif Hedstrom <zw...@apache.org> wrote:
> > >
> > > We have this API:
> > >
> > > void TSHttpTxnSetHttpRetStatus(TSHttpTxn txnp, TSHttpStatus
> > http_retstatus);
> > >
> > >
> > > Which on its own is a little strange, but looking at code, and talking
> > to Alan, I believe this was added when we added remap plugins long ago,
> to
> > allow such plugins to modify status codes without having an HttpHdr
> > response.
> > >
> > > So, I guess that is fine, and we use it fairly frequently, but looking
> > at this, the naming is really non-standard. I’d like to propose that we
> > change this API to
> > >
> > > void TSHttpTxnStatusSet(TSHttpTxn txnp, TSHttpStatus status);
> > >
> > >
> > >
> > > This seems much more inline with the other StatusSet that we have,
> > TSHttpHdrStatusSet().
> > >
> > >
> > > If this is reasonable, I’d also like to add a
> > TSHttpTxnStatusGet(TSHttpTxn txnp) call.
> > >
> > >
> > > In addition, I’m contemplating adding two new APIs, on the same note:
> > >
> > > void TSHttpTxnStatusSet(TSHttpTxn txnp, const char* reason);
> > > const char* TSHttpTxnStatusGet(TSHttpTxn txnp);
> >
> >
> >
> > Sigh, the last two should be
> >
> > void TSHttpTxnReasonSet(TSHttpTxn txnp, const char* reason);
> > const char* TSHttpTxnReasonGet(TSHttpTxn txnp);
> >
> >
> > The point being, I might want to set the transaction return code to a 403
> > in a remap plugin, but there’s no way to set a custom reason phrase right
> > now.
> >
> > Cheers,
> >
> > — leif
> >
> >
> >
>
> --
> *Beware the fisherman who's casting out his line in to a dried up
> riverbed.*
> *Oh don't try to tell him 'cause he won't believe. Throw some bread to the
> ducks instead.*
> *It's easier that way. *- Genesis : Duke : VI 25-28
>
--
Derek
Re: [PROPOSAL] Rename TSHttpTxnSetHttpRetStatus()
Posted by Alan Carroll <so...@oath.com.INVALID>.
+1
On Mon, Aug 6, 2018 at 4:18 PM Leif Hedstrom <zw...@apache.org> wrote:
>
>
> > On Aug 6, 2018, at 3:14 PM, Leif Hedstrom <zw...@apache.org> wrote:
> >
> > We have this API:
> >
> > void TSHttpTxnSetHttpRetStatus(TSHttpTxn txnp, TSHttpStatus
> http_retstatus);
> >
> >
> > Which on its own is a little strange, but looking at code, and talking
> to Alan, I believe this was added when we added remap plugins long ago, to
> allow such plugins to modify status codes without having an HttpHdr
> response.
> >
> > So, I guess that is fine, and we use it fairly frequently, but looking
> at this, the naming is really non-standard. I’d like to propose that we
> change this API to
> >
> > void TSHttpTxnStatusSet(TSHttpTxn txnp, TSHttpStatus status);
> >
> >
> >
> > This seems much more inline with the other StatusSet that we have,
> TSHttpHdrStatusSet().
> >
> >
> > If this is reasonable, I’d also like to add a
> TSHttpTxnStatusGet(TSHttpTxn txnp) call.
> >
> >
> > In addition, I’m contemplating adding two new APIs, on the same note:
> >
> > void TSHttpTxnStatusSet(TSHttpTxn txnp, const char* reason);
> > const char* TSHttpTxnStatusGet(TSHttpTxn txnp);
>
>
>
> Sigh, the last two should be
>
> void TSHttpTxnReasonSet(TSHttpTxn txnp, const char* reason);
> const char* TSHttpTxnReasonGet(TSHttpTxn txnp);
>
>
> The point being, I might want to set the transaction return code to a 403
> in a remap plugin, but there’s no way to set a custom reason phrase right
> now.
>
> Cheers,
>
> — leif
>
>
>
--
*Beware the fisherman who's casting out his line in to a dried up riverbed.*
*Oh don't try to tell him 'cause he won't believe. Throw some bread to the
ducks instead.*
*It's easier that way. *- Genesis : Duke : VI 25-28
Re: [PROPOSAL] Rename TSHttpTxnSetHttpRetStatus()
Posted by Leif Hedstrom <zw...@apache.org>.
> On Aug 6, 2018, at 3:14 PM, Leif Hedstrom <zw...@apache.org> wrote:
>
> We have this API:
>
> void TSHttpTxnSetHttpRetStatus(TSHttpTxn txnp, TSHttpStatus http_retstatus);
>
>
> Which on its own is a little strange, but looking at code, and talking to Alan, I believe this was added when we added remap plugins long ago, to allow such plugins to modify status codes without having an HttpHdr response.
>
> So, I guess that is fine, and we use it fairly frequently, but looking at this, the naming is really non-standard. I’d like to propose that we change this API to
>
> void TSHttpTxnStatusSet(TSHttpTxn txnp, TSHttpStatus status);
>
>
>
> This seems much more inline with the other StatusSet that we have, TSHttpHdrStatusSet().
>
>
> If this is reasonable, I’d also like to add a TSHttpTxnStatusGet(TSHttpTxn txnp) call.
>
>
> In addition, I’m contemplating adding two new APIs, on the same note:
>
> void TSHttpTxnStatusSet(TSHttpTxn txnp, const char* reason);
> const char* TSHttpTxnStatusGet(TSHttpTxn txnp);
Sigh, the last two should be
void TSHttpTxnReasonSet(TSHttpTxn txnp, const char* reason);
const char* TSHttpTxnReasonGet(TSHttpTxn txnp);
The point being, I might want to set the transaction return code to a 403 in a remap plugin, but there’s no way to set a custom reason phrase right now.
Cheers,
— leif