Fix Kafka Client leaks in the SampleFilterIntegrationTest.

[k-wall]

Type of change

• Bugfix

Description

Fix Kafka Client leaks in the SampleFilterIntegrationTest. Closing the test doesn't close the admin/consumer/producers
that it created.

Additional Context

Why are you making this pull request?

Checklist

Please go through this checklist and make sure all applicable tasks have been done

• Write tests
• Make sure all tests pass
• Review performance test results. Ensure that any degradations to performance numbers are understood and justified.
• Make sure all Sonar-Lint warnings are addressed or are justifiably ignored.
• Update documentation
• Reference relevant issue(s) and close them after merging
• For user facing changes, update CHANGELOG.md (remember to include changes affecting the API of the test artefacts too).

[SamBarker]

LGTM

[SamBarker]

Im happy with this as a tactical choice, but longer term? Would a better fix not be to have test.close() ensure all the clients it has handed out are closed?

I also wondered about making FilterIntegrationTest.admin etc some sort of lazy function

[gracegrimwood]

I had assumed (incorrectly) that tester.close() also closed everything that had been created by the tester (i.e. admin, producer, consumer). Having the tester check what it created and then ensure they're all correctly disposed of on close makes the most sense to me.

[k-wall]

Sometimes we want the lifespan of the Kafka object (producer, consumer, admin) to extend beyond the lifespan of Kroxylicious instance. We use this so that we can ensure that clients are capable of reconnecting through a replacement instance.

One way to solve this problem might be to break the association between the lifecycle of the tester the lifecycle of the kroxylicious. That would retain the flexibility we need to write this sort of test:

try(var tester = kroxyliciousTester();// No longer creates a kroxylicious instance
     var admin = tester.admin()) { 
     try(var kroxy = tester.kroxylicious()) { // Creates a Kroxylicious
        // ...
        //  do something with admin
     } 
     try(var kroxy = tester.kroxylicious()) { // Creates a Kroxylicious
        // ...
        // check admin reconnects
     }
}

It would also mean tester.close() could be given the semantics to ensure that everything it created has been closed, which is more obvious for the programmer to reason about.

[k-wall]

WDYT? @robobario @gracegrimwood @SamBarker ?

[SamBarker]

Yeah that sounds like a good way to go. I've raised that proposal as #526

[robobario]

Does that conflict with your other issue #378, if we can make clients independent of the existence of a Kroxylicious instance?

Another way would be to make KroxyliciousTester able to clean up dangling clients on close, and in special cases like the resilience test we construct them in the test using config supplied by the tester so they are detached from Tester.

moved discussion to issue #526

[gracegrimwood]

Nice catch! LGTM

[gracegrimwood]

I had assumed (incorrectly) that tester.close() also closed everything that had been created by the tester (i.e. admin, producer, consumer). Having the tester check what it created and then ensure they're all correctly disposed of on close makes the most sense to me.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication