chia7712 commented on code in PR #16356: URL: https://github.com/apache/kafka/pull/16356#discussion_r1642920606
########## streams/src/test/java/org/apache/kafka/streams/state/internals/AbstractRocksDBSessionStoreTest.java: ########## @@ -0,0 +1,75 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.streams.state.internals; + +import org.apache.kafka.common.serialization.Serde; +import org.apache.kafka.streams.state.SessionStore; +import org.apache.kafka.streams.state.Stores; + +import static java.time.Duration.ofMillis; + +public abstract class AbstractRocksDBSessionStoreTest extends AbstractSessionBytesStoreTest { + + private static final String STORE_NAME = "rocksDB session store"; + + public abstract StoreType storeType(); Review Comment: please remove `public` ########## streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBTimeOrderedSessionStoreWithIndexTest.java: ########## @@ -0,0 +1,25 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.streams.state.internals; + +public class RocksDBTimeOrderedSessionStoreWithIndexTest extends AbstractRocksDBSessionStoreTest { + + @Override + public StoreType storeType() { Review Comment: please remove `public` ########## streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBTimeOrderedWindowSchemaWithIndexSegmentedBytesStoreTest.java: ########## @@ -0,0 +1,30 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.streams.state.internals; + +public class RocksDBTimeOrderedWindowSchemaWithIndexSegmentedBytesStoreTest extends AbstractRocksDBTimeOrderedWindowSegmentedBytesStoreTest { + + @Override + protected boolean hasIndex() { + return true; + } + + @Override + protected AbstractRocksDBTimeOrderedWindowSegmentedBytesStoreTest.SchemaType schemaType() { Review Comment: please remove "protected" ########## streams/src/test/java/org/apache/kafka/streams/state/internals/AbstractRocksDBTimeOrderedWindowSegmentedBytesStoreTest.java: ########## @@ -16,59 +16,37 @@ */ package org.apache.kafka.streams.state.internals; -import static java.util.Arrays.asList; - -import java.util.Collection; import org.apache.kafka.streams.state.internals.PrefixedSessionKeySchemas.KeyFirstSessionKeySchema; import org.apache.kafka.streams.state.internals.PrefixedSessionKeySchemas.TimeFirstSessionKeySchema; import org.apache.kafka.streams.state.internals.PrefixedWindowKeySchemas.KeyFirstWindowKeySchema; import org.apache.kafka.streams.state.internals.PrefixedWindowKeySchemas.TimeFirstWindowKeySchema; import org.apache.kafka.streams.state.internals.SegmentedBytesStore.KeySchema; -import org.junit.runner.RunWith; -import org.junit.runners.Parameterized; -@RunWith(Parameterized.class) -public class RocksDBTimeOrderedWindowSegmentedBytesStoreTest +public abstract class AbstractRocksDBTimeOrderedWindowSegmentedBytesStoreTest extends AbstractDualSchemaRocksDBSegmentedBytesStoreTest<KeyValueSegment> { private static final String METRICS_SCOPE = "metrics-scope"; - private enum SchemaType { + protected enum SchemaType { Review Comment: please use package-private ########## streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBTimeOrderedSessionSchemaWithoutIndexSegmentedBytesStoreTest.java: ########## @@ -0,0 +1,30 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.streams.state.internals; + +public class RocksDBTimeOrderedSessionSchemaWithoutIndexSegmentedBytesStoreTest extends AbstractRocksDBTimeOrderedWindowSegmentedBytesStoreTest { + + @Override + protected boolean hasIndex() { Review Comment: please remove "protected" ########## streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBSessionStoreTest.java: ########## @@ -16,78 +16,10 @@ */ package org.apache.kafka.streams.state.internals; -import java.util.Collection; -import org.apache.kafka.common.serialization.Serde; -import org.apache.kafka.streams.state.SessionStore; -import org.apache.kafka.streams.state.Stores; - -import org.junit.runner.RunWith; -import org.junit.runners.Parameterized; -import org.junit.runners.Parameterized.Parameter; - -import static java.time.Duration.ofMillis; -import static java.util.Arrays.asList; - -@RunWith(Parameterized.class) -public class RocksDBSessionStoreTest extends AbstractSessionBytesStoreTest { - - private static final String STORE_NAME = "rocksDB session store"; - - @Parameter - public StoreType storeType; - - @Parameterized.Parameters(name = "{0}") - public static Collection<Object[]> getParamStoreType() { - return asList(new Object[][] { - {StoreType.RocksDBSessionStore}, - {StoreType.RocksDBTimeOrderedSessionStoreWithIndex}, - {StoreType.RocksDBTimeOrderedSessionStoreWithoutIndex} - }); - } +public class RocksDBSessionStoreTest extends AbstractRocksDBSessionStoreTest { @Override - StoreType getStoreType() { - return storeType; + public StoreType storeType() { Review Comment: please remove `public` ########## streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBTimeOrderedSessionStoreWithoutIndexTest.java: ########## @@ -0,0 +1,25 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.streams.state.internals; + +public class RocksDBTimeOrderedSessionStoreWithoutIndexTest extends AbstractRocksDBSessionStoreTest { + + @Override + public StoreType storeType() { Review Comment: please remove `public` ########## streams/src/test/java/org/apache/kafka/streams/state/internals/AbstractRocksDBTimeOrderedWindowSegmentedBytesStoreTest.java: ########## @@ -16,59 +16,37 @@ */ package org.apache.kafka.streams.state.internals; -import static java.util.Arrays.asList; - -import java.util.Collection; import org.apache.kafka.streams.state.internals.PrefixedSessionKeySchemas.KeyFirstSessionKeySchema; import org.apache.kafka.streams.state.internals.PrefixedSessionKeySchemas.TimeFirstSessionKeySchema; import org.apache.kafka.streams.state.internals.PrefixedWindowKeySchemas.KeyFirstWindowKeySchema; import org.apache.kafka.streams.state.internals.PrefixedWindowKeySchemas.TimeFirstWindowKeySchema; import org.apache.kafka.streams.state.internals.SegmentedBytesStore.KeySchema; -import org.junit.runner.RunWith; -import org.junit.runners.Parameterized; -@RunWith(Parameterized.class) -public class RocksDBTimeOrderedWindowSegmentedBytesStoreTest +public abstract class AbstractRocksDBTimeOrderedWindowSegmentedBytesStoreTest extends AbstractDualSchemaRocksDBSegmentedBytesStoreTest<KeyValueSegment> { private static final String METRICS_SCOPE = "metrics-scope"; - private enum SchemaType { + protected enum SchemaType { WindowSchemaWithIndex, WindowSchemaWithoutIndex, SessionSchemaWithIndex, SessionSchemaWithoutIndex } - private boolean hasIndex; - private SchemaType schemaType; - - @Parameterized.Parameters(name = "{0}") - public static Collection<Object[]> getKeySchema() { - return asList(new Object[][] { - {SchemaType.WindowSchemaWithIndex, true}, - {SchemaType.WindowSchemaWithoutIndex, false}, - {SchemaType.SessionSchemaWithIndex, true}, - {SchemaType.SessionSchemaWithoutIndex, false} - }); - } - - public RocksDBTimeOrderedWindowSegmentedBytesStoreTest(final SchemaType schemaType, final boolean hasIndex) { - this.schemaType = schemaType; - this.hasIndex = hasIndex; - } - + protected abstract boolean hasIndex(); + protected abstract SchemaType schemaType(); Review Comment: please use package-private ########## streams/src/test/java/org/apache/kafka/streams/state/internals/KeyValueStoreBuilderTest.java: ########## @@ -47,27 +49,32 @@ public class KeyValueStoreBuilderTest { private KeyValueStore<Bytes, byte[]> inner; private KeyValueStoreBuilder<String, String> builder; - @Before - public void setUp() { - when(supplier.get()).thenReturn(inner); + public void setUpWithoutInner() { when(supplier.name()).thenReturn("name"); when(supplier.metricsScope()).thenReturn("metricScope"); builder = new KeyValueStoreBuilder<>( - supplier, - Serdes.String(), - Serdes.String(), - new MockTime() + supplier, Review Comment: please revert those unrelated changes ########## streams/src/test/java/org/apache/kafka/streams/state/internals/MeteredTimestampedKeyValueStoreTest.java: ########## @@ -106,35 +108,37 @@ public class MeteredTimestampedKeyValueStoreTest { private final Metrics metrics = new Metrics(); private Map<String, String> tags; - @Before - public void before() { + private void setUpWithoutContext() { mockTime = new MockTime(); metered = new MeteredTimestampedKeyValueStore<>( - inner, - "scope", - mockTime, - Serdes.String(), - new ValueAndTimestampSerde<>(Serdes.String()) + inner, Review Comment: please revert those changes ########## streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBTimeOrderedSessionSchemaWithoutIndexSegmentedBytesStoreTest.java: ########## @@ -0,0 +1,30 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.streams.state.internals; + +public class RocksDBTimeOrderedSessionSchemaWithoutIndexSegmentedBytesStoreTest extends AbstractRocksDBTimeOrderedWindowSegmentedBytesStoreTest { + + @Override + protected boolean hasIndex() { + return false; + } + + @Override + protected SchemaType schemaType() { Review Comment: please remove "protected" ########## streams/src/test/java/org/apache/kafka/streams/state/internals/AbstractRocksDBTimeOrderedWindowSegmentedBytesStoreTest.java: ########## @@ -16,59 +16,37 @@ */ package org.apache.kafka.streams.state.internals; -import static java.util.Arrays.asList; - -import java.util.Collection; import org.apache.kafka.streams.state.internals.PrefixedSessionKeySchemas.KeyFirstSessionKeySchema; import org.apache.kafka.streams.state.internals.PrefixedSessionKeySchemas.TimeFirstSessionKeySchema; import org.apache.kafka.streams.state.internals.PrefixedWindowKeySchemas.KeyFirstWindowKeySchema; import org.apache.kafka.streams.state.internals.PrefixedWindowKeySchemas.TimeFirstWindowKeySchema; import org.apache.kafka.streams.state.internals.SegmentedBytesStore.KeySchema; -import org.junit.runner.RunWith; -import org.junit.runners.Parameterized; -@RunWith(Parameterized.class) -public class RocksDBTimeOrderedWindowSegmentedBytesStoreTest +public abstract class AbstractRocksDBTimeOrderedWindowSegmentedBytesStoreTest extends AbstractDualSchemaRocksDBSegmentedBytesStoreTest<KeyValueSegment> { private static final String METRICS_SCOPE = "metrics-scope"; - private enum SchemaType { + protected enum SchemaType { WindowSchemaWithIndex, WindowSchemaWithoutIndex, SessionSchemaWithIndex, SessionSchemaWithoutIndex } - private boolean hasIndex; - private SchemaType schemaType; - - @Parameterized.Parameters(name = "{0}") - public static Collection<Object[]> getKeySchema() { - return asList(new Object[][] { - {SchemaType.WindowSchemaWithIndex, true}, - {SchemaType.WindowSchemaWithoutIndex, false}, - {SchemaType.SessionSchemaWithIndex, true}, - {SchemaType.SessionSchemaWithoutIndex, false} - }); - } - - public RocksDBTimeOrderedWindowSegmentedBytesStoreTest(final SchemaType schemaType, final boolean hasIndex) { - this.schemaType = schemaType; - this.hasIndex = hasIndex; - } - + protected abstract boolean hasIndex(); Review Comment: please use package-private ########## streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBTimeOrderedWindowSchemaWithoutIndexSegmentedBytesStoreTest.java: ########## @@ -0,0 +1,30 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.streams.state.internals; + +public class RocksDBTimeOrderedWindowSchemaWithoutIndexSegmentedBytesStoreTest extends AbstractRocksDBTimeOrderedWindowSegmentedBytesStoreTest { + + @Override + protected boolean hasIndex() { + return false; + } + + @Override + protected SchemaType schemaType() { Review Comment: please remove "protected" ########## streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBTimeOrderedWindowSchemaWithIndexSegmentedBytesStoreTest.java: ########## @@ -0,0 +1,30 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.streams.state.internals; + +public class RocksDBTimeOrderedWindowSchemaWithIndexSegmentedBytesStoreTest extends AbstractRocksDBTimeOrderedWindowSegmentedBytesStoreTest { + + @Override + protected boolean hasIndex() { Review Comment: please remove "protected" ########## streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBVersionedStoreSegmentValueFormatterTest.java: ########## @@ -18,61 +18,49 @@ import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.MatcherAssert.assertThat; -import static org.junit.Assert.assertThrows; +import static org.junit.jupiter.api.Assertions.assertThrows; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.stream.Stream; + import org.apache.kafka.streams.state.internals.RocksDBVersionedStoreSegmentValueFormatter.SegmentValue; import org.apache.kafka.streams.state.internals.RocksDBVersionedStoreSegmentValueFormatter.SegmentValue.SegmentSearchResult; -import org.junit.Test; -import org.junit.experimental.runners.Enclosed; -import org.junit.runner.RunWith; -import org.junit.runners.Parameterized; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; -@RunWith(Enclosed.class) public class RocksDBVersionedStoreSegmentValueFormatterTest { /** * Non-exceptional scenarios which are expected to occur during regular store operation. */ - @RunWith(Parameterized.class) public static class ExpectedCasesTest { Review Comment: Could you please extract all test cases? ########## streams/src/test/java/org/apache/kafka/streams/state/internals/TimestampedKeyValueStoreBuilderTest.java: ########## @@ -46,28 +48,33 @@ public class TimestampedKeyValueStoreBuilderTest { private RocksDBTimestampedStore inner; private TimestampedKeyValueStoreBuilder<String, String> builder; - @Before - public void setUp() { - when(supplier.get()).thenReturn(inner); + private void setUpWithoutInner() { when(supplier.name()).thenReturn("name"); when(supplier.metricsScope()).thenReturn("metricScope"); builder = new TimestampedKeyValueStoreBuilder<>( - supplier, - Serdes.String(), - Serdes.String(), - new MockTime() + supplier, Review Comment: please revert those unrelated changes ########## streams/src/test/java/org/apache/kafka/streams/state/internals/VersionedKeyValueStoreBuilderTest.java: ########## @@ -48,29 +50,34 @@ public class VersionedKeyValueStoreBuilderTest { private VersionedKeyValueStoreBuilder<String, String> builder; - @Before - public void setUp() { - when(supplier.get()).thenReturn(inner); + private void setUpWithoutInner() { when(supplier.name()).thenReturn(STORE_NAME); when(supplier.metricsScope()).thenReturn(METRICS_SCOPE); builder = new VersionedKeyValueStoreBuilder<>( - supplier, - Serdes.String(), - Serdes.String(), - new MockTime() + supplier, Review Comment: please revert those unrelated changes ########## streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBTimeOrderedWindowSchemaWithoutIndexSegmentedBytesStoreTest.java: ########## @@ -0,0 +1,30 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.streams.state.internals; + +public class RocksDBTimeOrderedWindowSchemaWithoutIndexSegmentedBytesStoreTest extends AbstractRocksDBTimeOrderedWindowSegmentedBytesStoreTest { + + @Override + protected boolean hasIndex() { Review Comment: please remove "protected" ########## streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBVersionedStoreSegmentValueFormatterTest.java: ########## @@ -280,38 +276,27 @@ public void shouldCreateNewWithRecord() { * inconsistency among segments. These tests verify that the store is able to recover * gracefully even if this happens. */ - @RunWith(Parameterized.class) public static class ExceptionalCasesTest { Review Comment: Could you please extract all test cases? -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
