Change in semantics of config.BufferLimit with addition of RequestAck in 1.4 about fluent-logger-golang HOT 5 CLOSED

The BufferLimit parameter is to configure the (max) number of pending messages to be sent. It's not the size of memory space. The change in #63 is intentional, and meaning was not changed. The original code make(chan []byte, num) is not to allocate a bytes array, but to allocate a channel for byte arrays.
We should add a clear document to describe it. But it's not a bug, nor an unintentional change.

from fluent-logger-golang.

Thanks for clarifying. I think clear documentation would be very helpful and appreciated here.

After looking more closely I think I might have misidentified where the change in meaning happened but this sort of seems like it's changing the meaning since before it was accepting []byte into appendBuffer() here and erroring if the len(new bytes) + len(pending bytes) > BufferLimit (which was presumably also in bytes?). Apologies if I'm reading this incorrectly. I want to be sure I understand the issue correctly so I can figure out and work on whatever fix is appropriate in the linked Moby issue.

Also, do you consider it a performance regression to pre-allocate 8x the memory with the same parameters post #63 as opposed to pre #63?

from fluent-logger-golang.

I checked the change you pointed (this). And yes, it changes the semantics of BufferLimit. It was the limit of actual memory space (the max length of []byte) but now it's the max number of pending []byte objects in the channel.
At the time when I reviewed that change, I thought it's ok because it also changes the default value (8M -> 8k).

I missed the case that users specify very large values like Gigabytes because it's just the on-memory pool, and its contents should be flushed to anywhere else as soon as possible (for example, Fluentd on other hosts). Logs on on-memory pools can be dropped easily due to many reasons (including host failure).

So, as my conclusion, it's a kind of regression, but it's not a serious problem because I expect it doesn't affect most users' use-cases.
Of course, I'll change my opinion if I know my idea was wrong... and I'll add clear descriptions about configuration parameters on README anyway.

@IRCody does it answer to your question?

from fluent-logger-golang.

Yes it does. thanks @tagomoris. I agree it's sort of an outlier case.

My prompting for making this issue was the linked issue where dockerd's fluent logdriver. My understanding was that they wanted confirmation that this was indeed a change made in this library to inform the fix that would be made in the docker logdriver. Thanks for acknowledging the change here. I will update that ticket here to see how they want to proceed.

from fluent-logger-golang.

Let me close this ticket. And feel free to reopen if other comments / any commits are needed.

from fluent-logger-golang.