Send User-Agent as the monitor info #2154
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abh3
approved these changes
Jan 11, 2024
|
Looks like this breaks HTTP transfers. I will need to revert and make another release. Here are excerpts from the logs without this change: and then with this change: @bbockelm Did you test locally the change when proposing the pull request? |
|
@bbockelm Could you look at this again and see why this is happening? |
|
@abh3 Cedric already created a pull request with a fix, which I confirmed and merged into |
|
Then whose problem was it and what was the problem?
…________________________________
From: Guilherme Amadio ***@***.***>
Sent: Wednesday, January 24, 2024 11:02 PM
To: xrootd/xrootd ***@***.***>
Cc: Andrew Hanushevsky ***@***.***>; Mention ***@***.***>
Subject: Re: [xrootd/xrootd] Send User-Agent as the monitor info (PR #2154)
@abh3<https://github.com/abh3> Cedric already created a pull request with a fix, which I confirmed and merged into devel.
—
Reply to this email directly, view it on GitHub<#2154 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAUIW573JBT5367JNQ3C74DYQH7QVAVCNFSM6AAAAABAXFRMNSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBZGQ3DMMJXHE>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Hi @abh3, Brian used the Bridge to set the user-agent for the transfer and did not handle that case in the callback that is ran once the bridge executed the command. This prevented the file |
|
The problem was introduced by this pull request, and after I found out it broke http transfers, Cedric looked at it and fixed it. Today I'm going to make another release (5.6.6) including the fix. |
|
Hi Guilherme,
So, clearly, he never tested the change, sigh. OK, negative points here which means he gets lower priority unless we are assured the submitted patch has been tested. Perhaps even requiring a description on how to test the change so we can verify (that's the extreme case).
Andy
…________________________________
From: Guilherme Amadio ***@***.***>
Sent: Thursday, January 25, 2024 12:23 AM
To: xrootd/xrootd ***@***.***>
Cc: Andrew Hanushevsky ***@***.***>; Mention ***@***.***>
Subject: Re: [xrootd/xrootd] Send User-Agent as the monitor info (PR #2154)
The problem was introduced by this pull request, and after I found out it broke http transfers, Cedric looked at it and fixed it. Today I'm going to make another release (5.6.6) including the fix.
—
Reply to this email directly, view it on GitHub<#2154 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAUIW57Q5XFWQPWYYYGVEBLYQIJBXAVCNFSM6AAAAABAXFRMNSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBZGY2DANBXGM>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
My plan is to expand as much as possible our test suite to cover the most common operations such that we catch problems with patches when the pull request is opened. In #2134, the problem was caught by the new CI, but not the old. I am working on adding tests with http(s) transfers and a few authentication methods for 5.7. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After the initial login to the xrootd bridge (and after parsing the first request's headers), perform a
kXR_setfor the monitoring info. TheUser-Agentheader is sent as the info.This is only done for the first request of a connection. It is assumed that all subsequent connections belong to the same user agent.