rpuch commented on code in PR #5081: URL: https://github.com/apache/ignite-3/pull/5081#discussion_r1924804117
########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/benchmark/CreatingDistributionZoneBenchmark.java: ########## @@ -0,0 +1,115 @@ +/* + * 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.ignite.internal.benchmark; + +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static org.apache.ignite.internal.catalog.CatalogService.DEFAULT_STORAGE_PROFILE; +import static org.openjdk.jmh.annotations.Mode.AverageTime; + +import java.util.concurrent.atomic.AtomicInteger; +import org.apache.ignite.catalog.definitions.ZoneDefinition; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.RunnerException; +import org.openjdk.jmh.runner.options.Options; +import org.openjdk.jmh.runner.options.OptionsBuilder; + +/** + * Benchmark that measures creating a new empty distribution zone. + * The word `empty` means that the zone has no tables. + */ +@Fork(1) +@State(Scope.Benchmark) +public class CreatingDistributionZoneBenchmark extends AbstractMultiNodeBenchmark { + @Param({"3"}) + private int clusterSize; + + @Param({"1", "8"}) + private int partitionCount; + + @Param({"1", "3"}) + private int replicaCount; + + /** Distribution zones counter. */ + private AtomicInteger cnt = new AtomicInteger(); + + @Override + protected int nodes() { + return clusterSize; + } + + @Override + protected int partitionCount() { + return partitionCount; + } + + @Override + protected int replicaCount() { + return replicaCount; + } + + @Override + protected void createDefaultZoneOnStartup() { + // There is no need to create a zone on start-up. + } + + @Override + protected void createDefaultTableOnStartup() { + // There is no need to create a table on start-up. + } + + /** + * Measures creating a new empty distribution zone. The word `empty` means that the zone has no tables. + */ + @Benchmark + @Threads(1) + @Warmup(iterations = 5, time = 5) + @Measurement(iterations = 5, time = 5) + @BenchmarkMode(AverageTime) + @OutputTimeUnit(MILLISECONDS) + public void createEmptyDistributionZone() { + ZoneDefinition zone = ZoneDefinition.builder("zone_test_" + cnt.incrementAndGet()) + .ifNotExists() Review Comment: Why do we need this? Could it mask a programming error if we try to create a zone that already exists? ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/benchmark/AbstractMultiNodeBenchmark.java: ########## @@ -90,25 +90,35 @@ public void nodeSetUp() throws Exception { startCluster(); try { - var queryEngine = igniteImpl.queryEngine(); + // Create a default zone on the cluster's start-up. + createDefaultZoneOnStartup(); Review Comment: It seems there is a clash in terminology. There is a default zone that is created by the cluster on initialization automatically (implicitly). Here, another zone is created explicitly, so it probably should not be named a 'default zone'. How about just `createZoneOnStartup()`? ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/benchmark/CreatingTableBenchmark.java: ########## @@ -0,0 +1,104 @@ +/* + * 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.ignite.internal.benchmark; + +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static org.openjdk.jmh.annotations.Mode.AverageTime; + +import java.util.concurrent.atomic.AtomicInteger; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.RunnerException; +import org.openjdk.jmh.runner.options.Options; +import org.openjdk.jmh.runner.options.OptionsBuilder; + +/** + * This benchmark measures creating a new table in the default distribution zone. + */ +@Fork(1) +@State(Scope.Benchmark) +public class CreatingTableBenchmark extends AbstractMultiNodeBenchmark { + @Param({"3"}) + private int clusterSize; + + @Param({"1", "8"}) + private int partitionCount; + + @Param({"1", "3"}) + private int replicaCount; + + /** Tables counter. */ + private AtomicInteger cnt = new AtomicInteger(); + + @Override + protected int nodes() { + return clusterSize; + } + + @Override + protected int partitionCount() { + return partitionCount; + } + + @Override + protected int replicaCount() { + return replicaCount; + } + + @Override + protected void createDefaultTableOnStartup() { + // There is no need to create a table on start-up. + } + + /** + * Measures creating a new table in the default distribution zone. + */ + @Benchmark + @Threads(1) + @Warmup(iterations = 5, time = 5) + @Measurement(iterations = 5, time = 5) + @BenchmarkMode(AverageTime) + @OutputTimeUnit(MILLISECONDS) + public void createTableInDefaultZone() { Review Comment: How about having a boolean parameter that would tell whether a put should be made or not? We would be able to see the gap between creating an empty table and it becoming ready for puts. ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/benchmark/CreatingTableBenchmark.java: ########## @@ -0,0 +1,104 @@ +/* + * 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.ignite.internal.benchmark; + +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static org.openjdk.jmh.annotations.Mode.AverageTime; + +import java.util.concurrent.atomic.AtomicInteger; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.RunnerException; +import org.openjdk.jmh.runner.options.Options; +import org.openjdk.jmh.runner.options.OptionsBuilder; + +/** + * This benchmark measures creating a new table in the default distribution zone. + */ +@Fork(1) +@State(Scope.Benchmark) +public class CreatingTableBenchmark extends AbstractMultiNodeBenchmark { + @Param({"3"}) + private int clusterSize; + + @Param({"1", "8"}) + private int partitionCount; + + @Param({"1", "3"}) + private int replicaCount; + + /** Tables counter. */ + private AtomicInteger cnt = new AtomicInteger(); + + @Override + protected int nodes() { + return clusterSize; + } + + @Override + protected int partitionCount() { + return partitionCount; + } + + @Override + protected int replicaCount() { + return replicaCount; + } + + @Override + protected void createDefaultTableOnStartup() { + // There is no need to create a table on start-up. + } + + /** + * Measures creating a new table in the default distribution zone. + */ + @Benchmark + @Threads(1) + @Warmup(iterations = 5, time = 5) + @Measurement(iterations = 5, time = 5) + @BenchmarkMode(AverageTime) + @OutputTimeUnit(MILLISECONDS) + public void createTableInDefaultZone() { + String tableName = "table_test_" + cnt.incrementAndGet(); + + createTable(tableName); Review Comment: This method should be switched to public API as otherwise it's not clear what we measure here ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/benchmark/CreatingDistributionZoneBenchmark.java: ########## @@ -0,0 +1,115 @@ +/* + * 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.ignite.internal.benchmark; + +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static org.apache.ignite.internal.catalog.CatalogService.DEFAULT_STORAGE_PROFILE; +import static org.openjdk.jmh.annotations.Mode.AverageTime; + +import java.util.concurrent.atomic.AtomicInteger; +import org.apache.ignite.catalog.definitions.ZoneDefinition; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.RunnerException; +import org.openjdk.jmh.runner.options.Options; +import org.openjdk.jmh.runner.options.OptionsBuilder; + +/** + * Benchmark that measures creating a new empty distribution zone. + * The word `empty` means that the zone has no tables. + */ +@Fork(1) +@State(Scope.Benchmark) +public class CreatingDistributionZoneBenchmark extends AbstractMultiNodeBenchmark { + @Param({"3"}) + private int clusterSize; + + @Param({"1", "8"}) + private int partitionCount; + + @Param({"1", "3"}) + private int replicaCount; + + /** Distribution zones counter. */ + private AtomicInteger cnt = new AtomicInteger(); Review Comment: ```suggestion private final AtomicInteger cnt = new AtomicInteger(); ``` ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/benchmark/CreatingDistributionZoneBenchmark.java: ########## @@ -0,0 +1,115 @@ +/* + * 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.ignite.internal.benchmark; + +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static org.apache.ignite.internal.catalog.CatalogService.DEFAULT_STORAGE_PROFILE; +import static org.openjdk.jmh.annotations.Mode.AverageTime; + +import java.util.concurrent.atomic.AtomicInteger; +import org.apache.ignite.catalog.definitions.ZoneDefinition; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.RunnerException; +import org.openjdk.jmh.runner.options.Options; +import org.openjdk.jmh.runner.options.OptionsBuilder; + +/** + * Benchmark that measures creating a new empty distribution zone. + * The word `empty` means that the zone has no tables. + */ +@Fork(1) +@State(Scope.Benchmark) +public class CreatingDistributionZoneBenchmark extends AbstractMultiNodeBenchmark { Review Comment: `AbstractMultiNodeBenchmark` inits cluster with `TestIgnitionManager.init()`, which uses test defaults for things like delayDuration, idleSafeTimePropagationInterval, maxClockSkew. Test defaults for these values are ridiculously low; benchmarking with them has some value as it allows to (almost) exclude waits imposed by the schema sync protocol. But maybe we also need to benchmark with real defaults? There is a crutch: you can pass `TestIgnitionManager#PRODUCTION_CLUSTER_CONFIG_STRING` as cluster config to instruct `TestIgnitionManager#init()` to NOT apply test defaults. Maybe we could have a boolean parameter in the benchmark, like `tinySchemaSyncWaits`? If it is `true`, we could keep current behavior; otherwise, we could use production defaults to make schema sync look real. ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/benchmark/AbstractMultiNodeBenchmark.java: ########## @@ -90,25 +90,35 @@ public void nodeSetUp() throws Exception { startCluster(); try { - var queryEngine = igniteImpl.queryEngine(); + // Create a default zone on the cluster's start-up. + createDefaultZoneOnStartup(); - var createZoneStatement = "CREATE ZONE IF NOT EXISTS " + ZONE_NAME + " WITH partitions=" + partitionCount() - + ", replicas=" + replicaCount() + ", storage_profiles ='" + DEFAULT_STORAGE_PROFILE + "'"; - - getAllFromCursor( - await(queryEngine.queryAsync( - SqlPropertiesHelper.emptyProperties(), igniteImpl.observableTimeTracker(), null, null, createZoneStatement - )) - ); - - createTable(TABLE_NAME); + // Create a default table on the cluster's start-up. + createDefaultTableOnStartup(); } catch (Throwable th) { nodeTearDown(); throw th; } } + protected void createDefaultZoneOnStartup() { + var queryEngine = igniteImpl.queryEngine(); + + var createZoneStatement = "CREATE ZONE IF NOT EXISTS " + ZONE_NAME + " WITH partitions=" + partitionCount() + + ", replicas=" + replicaCount() + ", storage_profiles ='" + DEFAULT_STORAGE_PROFILE + "'"; + + getAllFromCursor( Review Comment: I wonder why we don't create a zone via public SQL API. Does it make sense to ask the guys who wrote this initially, and if there is no good reason for this, to switch to public API usage? It would make it more difficult to break something later accidentally ########## modules/runner/src/integrationTest/java/org/apache/ignite/internal/benchmark/CreatingTableBenchmark.java: ########## @@ -0,0 +1,104 @@ +/* + * 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.ignite.internal.benchmark; + +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static org.openjdk.jmh.annotations.Mode.AverageTime; + +import java.util.concurrent.atomic.AtomicInteger; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.RunnerException; +import org.openjdk.jmh.runner.options.Options; +import org.openjdk.jmh.runner.options.OptionsBuilder; + +/** + * This benchmark measures creating a new table in the default distribution zone. + */ +@Fork(1) +@State(Scope.Benchmark) +public class CreatingTableBenchmark extends AbstractMultiNodeBenchmark { Review Comment: Same thing about test defaults -- 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: notifications-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
