Conversation
531a956 to
81d32d8
Compare
|
/assign @thaJeztah |
fifo.go
Outdated
| const ( | ||
| F_SETPIPE_SZ = 1031 | ||
| ) | ||
|
|
||
| const FIFO_SIZE = 1024 * 1024 |
There was a problem hiding this comment.
GitHub actions looks to have purged logs for the previous run, but I think the linter may have been complaining about this, and likely wants them to be combined;
const (
F_SETPIPE_SZ = 1031
FIFO_SIZE = 1024 * 1024
)And as both are only used internally, it's better to not export them; perhaps use camelCase to make linters happy, but you can refer to the upstream counterpart for these, e.g.
// Sizes here match what's defined in the kernel; see https://github.com/torvalds/linux/commit/ff9da691c0498ff81fdd014e7a0731dab2337dac
const (
fsetPipeSize = 1031 // F_SETPIPE_SZ
fifoSize = 1024 * 1024 // FIFO_SIZE
)I'm also curios if these are consts that would make sense to be included in golang.org/x/sys - this PR is making changes assuming Linux, but the file is used for other unix-y platforms; would this be problematic for any of the other flavours (macOS, freeBSD)? /cc @samuelkarp
|
The failed tests seem not to be related to the change. |
Hm.. interesting; linting failure I suspect is because of an old version of GolangCI-lint, which is not yet aware of the Don't know about the other one (I'm not too familiar with all of this repository); |
|
Can you rebase your PR? #58 should have fixed the CI failures |
530c14d to
08074a4
Compare
Signed-off-by: kkkkun <scuzk373x@gmail.com>
Done. I do not understand the failed test. |
|
It's a shame that the api surface returns a ReadWriteCloser directly, otherwise I'd say why don't we just expose a method that lets you set the buffer size to whatever you want. Alternatively, we could expose a new constructor, like |
The container which writes logs synchronously would be blocked in the following scenarios:
So it is important to resize fifo in container runtime. After commit, the
pipe_max_sizehad adjusted to 1024K.