MINOR: code cleanup

[mjsax]

Similar to #6054

[mjsax]

Call for review @guozhangwang @bbejeck @vvcephei

[bbejeck]

retest this please

[mjsax]

Retest this please.

[vvcephei]

As before, thanks so much for doing this. I just had a few comments...

[vvcephei]

Do we need to relax this declaration?

[vvcephei]

@mjsax justified this in other PRs... Since the exceptions are unexpected, and they would cause the tests to fail if they happened anyway, detailed throws declarations just pollute the test method declarations.

[vvcephei]

Not your code, but double-brace initialization is not great.

We now have TestUtils.mkProperties to avoid it (in conjunction with mkMap and mkEntry)

Can we clean up the double-brace initializations here and elsewhere in this PR?

[mjsax]

Not your code, but double-brace initialization is not great.

Not sure why? But sure, can update.

[vvcephei]

Maybe it's not so important specifically in the context of tests, or with Properties specifically, but in general, there are two problems stemming from the fact that this is not just creating an instance; it's actually creating an instance of an anonymous subclass.

I've included some thoughts below, but this isn't just my opinion. Searching for "double-brace initialization" brings up several discussions about it as an antipattern, not just descriptions of what it is. I'm not sure I've covered all the bases, so those other pages might be worth a read if you like.

1:

Anonymous subclasses create garbage in the JVM. Maybe this is better in Java 11; I'm not sure. But I bet that anonymous subclasses are still heavier than instances. At a minimum, they close over their context, so there has to be a unique class sitting in the JVM for each invocation.

2:

Interacting with a superclass during initialization of a subclass is not the same as interacting with an instance of the class. It's close, but subtly different, which can lead to subtle bugs. For example, inside the double braces, you can invoke protected members by accident.

There's an extra dimension to this, since the superclass can change over time. The superclass may change public members to protected, and your code would still compile even though it shouldn't. Probably not such a concern for Properties, which hasn't changed since dinosaurs roamed the earth. But it's a reason to avoid the pattern in general.

Ideally, all classes that don't expect to be subclassed would declare themselves final, in which case the double-brace initialization wouldn't even work. But IMHO, it's better to be defensive and not expect authors of other classes to signal their intentions so clearly. In other words, I prefer to avoid subclassing classes unless I think they are really intended to be subclassed.

3:

More on the maintainability side: this pattern is a bit of an arcane invocation. There are plenty of Java developers who don't know you can do this, and plenty more who know about it but don't know why it works or what exactly it does. In other words, it's not clear if it meets the bar for simple, straightforward code that important infrastructure projects should adhere to.

For example, I'm not exactly a Java noob, but I still had to look up the initialization rules just now to make sure that the superclass's initialization blocks and constructor both get invoked before the subclass's initialization blocks.

It brings in a lot of concepts and complexity when all you really wanted to do was create a Properties inline. Why not just create and use a static method (like mkProperties) to do the same thing, since everyone has a very clear understanding of method invocation?

[vvcephei]

There's a heck of a lot of code inside the resource block of this try. The only thing that really needs to be there is the new KafkaStreams. Can we assign the config to a variable prior to the try block?

I don't feel strongly about this, but it just seems a little harder to parse visually when there is so much code separating the try from the actual block.

[vvcephei]

same as prior comments.

[vvcephei]

same comment as before.

[vvcephei]

I love this so much.

[guozhangwang]

Made a pass. Feel free to merge after getting to the comments.

[guozhangwang]

Why we want to generalize the exception to throw?

[mjsax]

Because it's the error case -- nobody is supposed to catch any exception and the test fails anyway. This is the way I was taught to write unit tests with good code style.

[guozhangwang]

Ditto here.

[guozhangwang]

And here.

[guozhangwang]

Elsewhere as well, will omit below.

[guozhangwang]

nit: I think it is not necessary to break to multiple lines.

[mjsax]

Updated.

[guozhangwang]

LGTM. Feel free to merge @mjsax

[mjsax]

Rebased to resolve merge conflict.