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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(inputs.http_listener_v2): Add unix socket mode #15764
feat(inputs.http_listener_v2): Add unix socket mode #15764
Changes from 10 commits
06bfdea
7cf3f65
20c253d
17eb6f5
c2b9157
95083f6
949e4a2
b958236
1427d39
bbad579
a239d9e
dfbaf83
45792b5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really necessary? Parsing a unix URL should also work on Windows, shouldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it was problematic as sample address will look like
unix://c:/Users/bazko1/temp/telegraf.socket
theurl.Parse
starts to interpret the part afterc:
as port and it results in an parse error.You can check that in
socket_listener
tests for windows are skipped with comment that the unixgram is not supported even though the unix socket is.See: https://github.com/influxdata/telegraf/blob/master/plugins/inputs/socket_listener/socket_listener_test.go#L125-L128
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the correct way is to use
unix:///C:/Users/bazko1/...
(i.e. three slashes) according to golang/go@844b625...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes great I tested it and this works I will remove the windows case then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is problematic in CI environments. Imagine another test using the same port, if they by chance run at the same time one of them will fail and we get flaky tests. I would remove the test here or solely stick to the URL parsing part without actually "starting" the listener...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree such test might be problematic when running in parallel with others, but afaik they are run synchronously and there are also other tests that actually, start listening on port.
Anyway my logic should be tested enough without
Start
as it is located inInit
function so I will remove theStart
method call.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well actually the
Init
already calls thenet.Listen
so that the port gets busy and we need to stop the internaldial.Listener
.I think my changes are big enough especially with special windows case that having unit test for url parsing would be nice so please reconsider the idea of fully removing test, but if you insist I will remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well starting the listener in
Init()
is also wrong, this should be done inStart()
... Anyhow, as you didn't introduce this, I don't blame you. ;-)Regarding the tests: Yes, the tests in this file/package are executes sequentially, but all packages are executed in parallel (in the best case)... So if e.g.
outputs.http
opens a server on this port we run into trouble... So it is acceptable to specify no port or port zero as this will allow the OS to choose a free port but the above will definitively bite us. Been there suffered that.In any case, what you do is to test if
url.Parse
works I think and I guess we should assume this works at least as long as this test will open a fixed port... So either change the test/code to not starting listen or remove the fixed ports.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay lets make it right then I agree that the test can be actually removed especially when windows unix path works fine with url.Parse.
I will move the
.Listen
part toStart
then.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 45792b5