You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "zwoop (via GitHub)" <gi...@apache.org> on 2023/03/08 02:07:18 UTC
[GitHub] [trafficserver] zwoop opened a new pull request, #9509: Reduce the size of the APIHooks, eliminating enum gap
zwoop opened a new pull request, #9509:
URL: https://github.com/apache/trafficserver/pull/9509
This is an ABI incompatible change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9509: Reduce the size of the APIHooks, eliminating enum gap
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9509:
URL: https://github.com/apache/trafficserver/pull/9509#discussion_r1131588336
##########
include/ts/apidefs.h.in:
##########
@@ -481,7 +481,7 @@ typedef enum {
// Putting the SSL hooks in the same enum space
// So both sets of hooks can be set by the same Hook function
- TS_SSL_FIRST_HOOK = 201,
+ TS_SSL_FIRST_HOOK,
Review Comment:
We talked about this on slack some. The tradeoff is this:
1. With a gap, we can add `TS_HTTP_*` values (hooks) without breaking compatibility.
2. Without a gap, any new hook breaks compatibility and thus must be done only on major releases.
If the decision is that we only add these hooks on major release, then we don't need a gap in these values. I'm fine with that. I just want to make sure that's understood.
---
Thus we can:
1. Remove the gap (as your original patch does) with the understanding that hooks can only be add on major releases.
2. Make the gap a smaller value to avoid the performance issue, but still keep the gap to allow for new hooks. Maybe leave a space of 5 values.
3. Refactor the enum into two enums so that the `TS_SSL*` hooks are in a separate enum. But note that the comment above these enums says that keeping them together in one enum is intentional.
I'm personally fine with any of these.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9509: Reduce the size of the APIHooks, eliminating enum gap
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9509:
URL: https://github.com/apache/trafficserver/pull/9509#discussion_r1132549626
##########
include/ts/apidefs.h.in:
##########
@@ -481,7 +481,7 @@ typedef enum {
// Putting the SSL hooks in the same enum space
// So both sets of hooks can be set by the same Hook function
- TS_SSL_FIRST_HOOK = 201,
+ TS_SSL_FIRST_HOOK,
Review Comment:
After some discussion on slack, my vote would be to:
1. Merge in your PR as is and get rid of the gap.
2. Look into refactoring the enum to separate out these two types (`TS_HTTP` and `TS_SSL`) so that the gap isn't needed. Let's keep that a separate PR if that can be done reasonably cleanly enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9509: Reduce the size of the APIHooks, eliminating enum gap
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9509:
URL: https://github.com/apache/trafficserver/pull/9509#discussion_r1131495746
##########
include/ts/apidefs.h.in:
##########
@@ -481,7 +481,7 @@ typedef enum {
// Putting the SSL hooks in the same enum space
// So both sets of hooks can be set by the same Hook function
- TS_SSL_FIRST_HOOK = 201,
+ TS_SSL_FIRST_HOOK,
Review Comment:
Or make the gap smaller? We only count to 20 now. Maybe set the `TS_SSL_FIRST_HOOK` to 30?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop merged pull request #9509: Reduce the size of the APIHooks, eliminating enum gap
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop merged PR #9509:
URL: https://github.com/apache/trafficserver/pull/9509
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a diff in pull request #9509: Reduce the size of the APIHooks, eliminating enum gap
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #9509:
URL: https://github.com/apache/trafficserver/pull/9509#discussion_r1131563387
##########
include/ts/apidefs.h.in:
##########
@@ -481,7 +481,7 @@ typedef enum {
// Putting the SSL hooks in the same enum space
// So both sets of hooks can be set by the same Hook function
- TS_SSL_FIRST_HOOK = 201,
+ TS_SSL_FIRST_HOOK,
Review Comment:
That's what the patch does, it makes the gap smaller by not fixing it at 201.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9509: Reduce the size of the APIHooks, eliminating enum gap
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9509:
URL: https://github.com/apache/trafficserver/pull/9509#discussion_r1131491907
##########
include/ts/apidefs.h.in:
##########
@@ -481,7 +481,7 @@ typedef enum {
// Putting the SSL hooks in the same enum space
// So both sets of hooks can be set by the same Hook function
- TS_SSL_FIRST_HOOK = 201,
+ TS_SSL_FIRST_HOOK,
Review Comment:
I see.
Can both the concerns be addressed by making the `TS_SSL_*` values a separate enum?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #9509: Reduce the size of the APIHooks, eliminating enum gap
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #9509:
URL: https://github.com/apache/trafficserver/pull/9509#issuecomment-1460345154
[approve ci]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #9509: Reduce the size of the APIHooks, eliminating enum gap
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #9509:
URL: https://github.com/apache/trafficserver/pull/9509#issuecomment-1460304142
[approve ci clang-format]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9509: Reduce the size of the APIHooks, eliminating enum gap
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9509:
URL: https://github.com/apache/trafficserver/pull/9509#discussion_r1131484156
##########
include/ts/apidefs.h.in:
##########
@@ -481,7 +481,7 @@ typedef enum {
// Putting the SSL hooks in the same enum space
// So both sets of hooks can be set by the same Hook function
- TS_SSL_FIRST_HOOK = 201,
+ TS_SSL_FIRST_HOOK,
Review Comment:
Maybe my intentions were misplaced, but I intentionally added this gap here to allow room for new `TS_HTTP_*` values without breaking compatibility:
https://github.com/apache/trafficserver/pull/8066
Note that any new `TS_HTTP_*` value requires a corresponding `TS_EVENT_HTTP_*` value at the same offset. But that's not the case for `TS_SSL_*` values. Therefore, any new `TS_HTTP_*` value added _must_ be added before the `TS_SSL_*` values, and therefore, without the gap, must break compatibility because all subsequent `TS_SSL` values would be shifted. This was not understood when someone added `TS_HTTP_REQUEST_BUFFER_READ_COMPLETE_HOOK` at the end of this enum after `TS_SSL_*`, which I'm sure was done to try to not break compatibility. I had to fix that in #8066.
Maybe allowing for new `TS_HTTP_*` values without breaking compatibility isn't valuable enough to justify the gap though. Can you explain what the problem is with having the gap?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9509: Reduce the size of the APIHooks, eliminating enum gap
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9509:
URL: https://github.com/apache/trafficserver/pull/9509#discussion_r1131487492
##########
include/ts/apidefs.h.in:
##########
@@ -481,7 +481,7 @@ typedef enum {
// Putting the SSL hooks in the same enum space
// So both sets of hooks can be set by the same Hook function
- TS_SSL_FIRST_HOOK = 201,
+ TS_SSL_FIRST_HOOK,
Review Comment:
See #9480 . This is the root of the performance win from that PR, which was discussed at the last PR review. In essence the gap creates a large number of elements in the area that get checked for being cleared but are never used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org