DEV Community

Cover image for My first Apache Kafka PR deleted three lines. I almost deleted the wrong one.
Ayoub Hammoudi
Ayoub Hammoudi

Posted on AI-assisted

My first Apache Kafka PR deleted three lines. I almost deleted the wrong one.

Deleting a duplicate line is the safest change in software. Nothing can break, because nothing changes. I believed that right up until I measured it.

The door

Apache Kafka does not run its CI for a first-time contributor. A pull request from someone the project has never merged gets a labeler and a triage bot, and the real build waits until a committer approves it. When I looked, one first-timer's pull request had been sitting for five days with two check runs, no comment and no review.

So a first pull request there is not a chance to impress anyone. It is a chance to be trusted once.

Before I found anything, I chased three families of defects and came back empty. A scanner for malformed SLF4J placeholders across 3,911 production files found zero. Leftover JUnit 4 imports: zero files. And a family of ByteBuffer.array() fixes, where five pull requests from three contributors were stuck on one question from a committer:

Did we verify if this is a real-world scenario before drafting this PR?

That question stayed with me for everything that followed.

The find

What survived was small. In build.gradle, three test dependencies were declared twice inside the same block:

core              libs.apachedsCoreApi        lines 1221 and 1230
connect:runtime   project(':server-common')   lines 3842 and 3847
connect:runtime   project(':server')          lines 3840 and 3848
Enter fullscreen mode Exit fullscreen mode

Gradle resolves a configuration as a set, so the second declaration does nothing. Delete it and move on.

Except that every pair has two lines, and you have to pick one.

The wrong one

For core I picked line 1221. It looked like the stray one, alone between two unrelated dependencies, while 1230 sat neatly at the head of the apacheds* group. Keeping 1230 made the file look tidier.

Then I did what that committer's question asks for. I generated the dependency report for both test configurations of both projects, before and after, and compared them.

The resolved sets were identical: 73, 104, 125 and 171 modules, not one difference.

The :core report was not. Fifteen lines had moved. org.apache.directory.api:api-all jumped from position 150 to 87, ahead of apacheds-core-api. Removing line 1221 had pushed apachedsCoreApi behind another declaration, and the report follows declaration order.

On the JVM, order is not cosmetic. When two jars on a classpath ship the same class, the first one wins. These two artifacts both come from Apache Directory and probably share no classes.

Probably is not a word I wanted to put in front of a reviewer.

The right one

So I removed the later line of each pair instead: 1230, 3847 and 3848. Every declaration that remains keeps its position, and the classpath order stays exactly as it was.

The four reports came back byte-identical, at 241, 418, 498 and 760 lines. Both modules compiled their tests, check passed without the test task, rat passed at the root, and WorkerTest ran 140 tests with no failures as a spot check on the connect side.

The description ended up far longer than the diff. One paragraph in it explains why the later line and not the earlier one. It answers the question before anyone has to ask it.

A committer approved it the next morning with two words, "Thanks, LGTM", and merged it a moment later.

What I took from it

A deletion is not a non-event. It has a position, and in a build file position is behaviour. "It resolves to the same set" and "it builds the same classpath" are two different claims, and only the first one was true of my first choice.

The other lesson is about proportion. In a project you have never touched, the size of the change and the size of the proof have nothing to do with each other. A three line diff with a boring, byte-identical proof is easy to merge. The same diff with "should be a no-op" is a question a busy committer has no time to answer.

The rule I keep now is simple. When you remove a duplicate declaration, remove the later one.

The pull request is apache/kafka#23786. How I look for these defects in the first place is in an earlier post.

I am Ayoub Hammoudi, a backend and AI platforms engineer in Paris. More of my work is at github.com/Ayoubhm07.

Top comments (1)

Collapse
 
abhinavs1920 profile image
Abhinav Sharma •

Removing the later duplicate is a good rule of thumb, the earlier declaration is usually the one other code already binds to. In a codebase the size of Kafka the wrong removal compiling cleanly is what makes it so easy to miss.