[FLINK-39761][runtime] Set 'Connection: close' header when sending '304 Not Modified' responses#28275
[FLINK-39761][runtime] Set 'Connection: close' header when sending '304 Not Modified' responses#28275nattilabalint wants to merge 1 commit into
Conversation
spuru9
left a comment
There was a problem hiding this comment.
Can you take a look at the comments. Thanks.
| public class StaticFileServerHandlerTest { | ||
|
|
||
| @Test | ||
| public void testSendNotModifiedIncludesConnectionCloseHeader() { |
There was a problem hiding this comment.
Test uses Mockito, which Flink forbids via checkstyle
https://flink.apache.org/how-to-contribute/code-style-and-quality-common/#avoid-mockito---use-reusable-test-implementations
| import org.junit.jupiter.api.Test; | ||
| import org.mockito.ArgumentCaptor; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; |
There was a problem hiding this comment.
The test correctly targets JUnit 5 (org.junit.jupiter), but two conventions are off for new Flink test code:
- Drop public on the class and the
@Testmethod — JUnit 5 doesn't require it and Flink's style drops it. - Use AssertJ, not Jupiter Assertions. Flink standardizes on AssertJ (assertThat(...)).
252b2a5 to
1cf1ced
Compare
…04 Not Modified' responses
1cf1ced to
3b2489f
Compare
|
@flinkbot run azure |
ferenc-csaky
left a comment
There was a problem hiding this comment.
Change LGTM, from a quick peek CI failure seems to be orthogonal, let's give that another try.
Before merge, pls. also open backport PRs for the fix versions defined in the corresponding jira.
spuru9
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments. LGTM.
What is the purpose of the change
Fix the proxy connection pool poisoning (putting back closed connections to the pool) on 304 Not Modified responses.
This is happening, because Flink severe the connections from its end when respond with HTTP_304, without explicitly setting the Connection: close header on the response.
Brief change log
The Connection: close header added to the response.
Verifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation
Was generative AI tooling used to co-author this PR?
Generated-by: Gemini 3.5