Fix incomplete log entries read by FramedInputStreamConsumer - #1754
Closed
yuting-liu wants to merge 11 commits into
Closed
Fix incomplete log entries read by FramedInputStreamConsumer#1754yuting-liu wants to merge 11 commits into
yuting-liu wants to merge 11 commits into
Conversation
Contributor
Author
Member
|
@yuting-liu thanks for submitting a PR! Could you please also add a unit test that reproduces the problem? I am not sure that this actually fixes anything, and, if it does, perhaps we could fix it without introducing an allocation per iteration |
Closed
Contributor
Author
|
@bsideup
do you have some advices on how to achieve it by without introducing an allocation per iteration. I'm currently planning to add the unit test in |
yuting-liu
force-pushed
the
yuting-fix-frame
branch
2 times, most recently
from
January 16, 2022 00:50
80dcf6e to
dee134d
Compare
yuting-liu
force-pushed
the
yuting-fix-frame
branch
from
January 16, 2022 00:51
dee134d to
66d7efd
Compare
yuting-liu
force-pushed
the
yuting-fix-frame
branch
from
January 16, 2022 00:54
92a2a73 to
905c8e9
Compare
Contributor
Author
|
@bsideup I added a unit test in in After the fix here, there is no such flakes issue for splitting messages. The real issue we countered that is that this flaky failure will break our docker log ingestion via docker-java and bring up troubles to our end users. |
Member
|
@yuting-liu Have you considered writing a unit test instead? It shouldn't be flaky because you will have more control and eliminate I/O from the test. Thanks! |
Contributor
Author
|
@bsideup The major issue is that after #1647, the log ingestion is not determined any more. The case of split logs are happened occasionally. Though the flakiness is quite stable that it's likely that if I run 10 times for the IT, there can be 8 or 9 times that 1 of two runs are failed. However, there can be some lucky case that the log message counts are as our expectation. If we want to change to ut, I have a few questions:
|
Member
We don't. We write a unit test for
We should write a test that always fails before your fix Thanks! |
Contributor
Author
|
@bsideup I tried a few times, but I feel that it's a bit complicated to add unit test on |
Contributor
Author
|
@bsideup can you help us to approve the change and make it into the next release then. |
Member
|
@yuting-liu sorry, not sure I understand the fix yet. The test you provided seems to be making an assumption that there will be a fixed number of frames, which might not be the case. Instead, we should verify that the streamed bytes are exactly as in the source. The current implementation seems to be handling it correctly, at least from the code PoV, so, unless you can provide a valid test that shows the problem, I do not see a bug yet. |
Contributor
Author
|
@bsideup Actually, that docker image I'm using is giving the same fixed output all the times, as following: Because the logs are stayed the same every time, it should be incorrect to have the list of frames changed every time. As we all agree that each frame should be each line of I also tried to write a unit test to directly use this log output instead of using the IT to calling the image. However, the way current I think this case should already demonstrate that there are some bugs in the buffer array used in |
Contributor
Author
|
@bsideup any comments on this PR? |
|
@bsideup This issue is blocking our customers. Could you please expedite the review and approval process? Appreciate your help here. |
Member
|
Hey @lei-sumo. Nothing changed since my last comment ( #1754 (comment) ). A test should be provided that will show that there is an actual issue, and so far I only saw wrong assumptions (frames are not guaranteed to arrive in full and should be glued). |
Contributor
Author
I don't think this is correct as our log ingestion relies on the thing that frames are guaranteed to arrive full and this seems to us it only gets changed after #1647. This means that this change(#1647) is not a backward compatible change. Besides, I think after we compare the test result between two test cases: #1782 and this one, we have already confirm that for the same test, if we can make the change in this PR, the assumption that frames can arrive full and no need to be glued then. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The issue was described in: #1647
After #1351, with the switch from
FrameStreamProcessortoFramedInputStreamConsumer, logs are no longer ingested into Frame with complete log messages.For the stream type of
RAW, the old frame stream processor useshttps://github.com/docker-java/docker-java/blob/master/docker-java-core/src/main/java/com/github/dockerjava/core/command/FrameReader.java. There the payload is reading in a buffer with payload size. We should keep the consistent behavior, otherwise the buffer of 1024 will lead to incomplete messages.