gharris1727 commented on code in PR #15910:
URL: https://github.com/apache/kafka/pull/15910#discussion_r1608959573
##########
connect/mirror/src/test/java/org/apache/kafka/connect/mirror/OffsetSyncStoreTest.java:
##########
@@ -82,77 +86,131 @@ public void testOffsetTranslation() {
@Test
public void testNoTranslationIfStoreNotStarted() {
- try (FakeOffsetSyncStore store = new FakeOffsetSyncStore()) {
- // no offsets exist and store is not started
- assertEquals(OptionalLong.empty(), store.translateDownstream(null,
tp, 0));
- assertEquals(OptionalLong.empty(), store.translateDownstream(null,
tp, 100));
- assertEquals(OptionalLong.empty(), store.translateDownstream(null,
tp, 200));
+ FakeOffsetSyncStore store = new FakeOffsetSyncStore() {
Review Comment:
I have somewhat strong feelings, I wouldn't call them very strong. If
someone noticed this IDE warning and created a ticket and a PR to fix it, I
would review that. Am I going to make the build enforce this warning? No, but I
have seen other situations where the warning did point out real resource
leaks...
I just wanted to save the effort required to go and rework this later, and
prevent this PR from introducing an easily avoidable warning. I agree with you
about suppressing warnings, I don't think that is a healthy practice to have.
I just tried making this a try-with-resources and the indenting turned out
fine. The body of backingStoreStart is at the exact same indentation as it is
currently.
```
try (FakeOffsetSyncStore store = new FakeOffsetSyncStore() {
@Override
void backingStoreStart() {
// read a sync during startup
sync(tp, 100, 200);
assertEquals(OptionalLong.empty(), translateDownstream(null,
tp, 0));
assertEquals(OptionalLong.empty(), translateDownstream(null,
tp, 100));
assertEquals(OptionalLong.empty(), translateDownstream(null,
tp, 200));
}
}) {
// no offsets exist and store is not started
assertEquals(OptionalLong.empty(),
store.translateDownstream(null, tp, 0));
assertEquals(OptionalLong.empty(),
store.translateDownstream(null, tp, 100));
assertEquals(OptionalLong.empty(),
store.translateDownstream(null, tp, 200));
// After the store is started all offsets are visible
store.start(true);
assertEquals(OptionalLong.of(-1),
store.translateDownstream(null, tp, 0));
assertEquals(OptionalLong.of(200),
store.translateDownstream(null, tp, 100));
assertEquals(OptionalLong.of(201),
store.translateDownstream(null, tp, 200));
}
```
Here's the Consumer alternative I thought about, which uses one less
indentation level at the cost of a variable, a field, and two constructors:
```
Consumer<FakeOffsetSyncStore> init = store -> {
// read a sync during startup
store.sync(tp, 100, 200);
assertEquals(OptionalLong.empty(),
store.translateDownstream(null, tp, 0));
assertEquals(OptionalLong.empty(),
store.translateDownstream(null, tp, 100));
assertEquals(OptionalLong.empty(),
store.translateDownstream(null, tp, 200));
};
try (FakeOffsetSyncStore store = new FakeOffsetSyncStore(init)) {
// no offsets exist and store is not started
assertEquals(OptionalLong.empty(),
store.translateDownstream(null, tp, 0));
assertEquals(OptionalLong.empty(),
store.translateDownstream(null, tp, 100));
assertEquals(OptionalLong.empty(),
store.translateDownstream(null, tp, 200));
// After the store is started all offsets are visible
store.start(true);
assertEquals(OptionalLong.of(-1),
store.translateDownstream(null, tp, 0));
assertEquals(OptionalLong.of(200),
store.translateDownstream(null, tp, 100));
assertEquals(OptionalLong.of(201),
store.translateDownstream(null, tp, 200));
}
```
Either of these is preferable to having the warning or suppressing it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
