paul-rogers commented on code in PR #13696: URL: https://github.com/apache/druid/pull/13696#discussion_r1084390936
########## integration-tests-ex/cases/cluster/Common/prepopulated-data.env: ########## @@ -0,0 +1,24 @@ +# +# 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. +# + +AWS_REGION=us-east-1 + +# Setting s3 credentials and region to use pre-populated data for testing. +druid_s3_accessKey=AKIAT2GGLKKJQCMG64V4 +druid_s3_secretKey=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 Review Comment: Do we want to check these keys into GitHub? If so, then we should have a comment that explains that, say, they point to a read-only public bucket, or whatever the story is. This will help folks who do a security scan and stumble across these keys. ########## integration-tests-ex/cases/cluster/Common/environment-configs/historical-for-query-error-test.env: ########## @@ -0,0 +1,32 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one Review Comment: Would be better if we put these files in a `QueryError` category to avoid cluttering `Common` with test-specific files. If we do that, then you need a query-specific `docker-compose.yaml` file. For this PR, that means copy/paste. Once the code gen PR is merged, it will mean a very short shell script. Is this file used by multiple tests? If it is, it makes sense to go into `Common`. We might choose a name that indicates the family of tests to which it applies, such as `historical-for-query-tests.env` or some such. ########## integration-tests-ex/cases/cluster/Query/prepopulated-data.env: ########## @@ -0,0 +1,24 @@ +# +# 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. +# + +AWS_REGION=us-east-1 + +# Setting s3 credentials and region to use pre-populated data for testing. +druid_s3_accessKey=AKIAT2GGLKKJQCMG64V4 +druid_s3_secretKey=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 Review Comment: This file looks identical to the one above in `Common`. ########## integration-tests-ex/cases/cluster/Query/docker-compose.yaml: ########## @@ -0,0 +1,98 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more Review Comment: The `cluster.sh` file says we're using `BatchIndex` for this test, yet we have a custom `docker-compose.yaml` file that isn't being used. ########## integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/query/ITQueryRetryTestOnMissingSegments.java: ########## @@ -0,0 +1,241 @@ +/* + * 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.druid.testsEx.query; + +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.inject.Inject; +import java.io.IOException; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.druid.java.util.common.ISE; +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.java.util.http.client.response.StatusResponseHolder; +import org.apache.druid.query.QueryContexts; +import org.apache.druid.server.coordination.ServerManagerForQueryErrorTest; +import org.apache.druid.testing.IntegrationTestingConfig; +import org.apache.druid.testing.clients.CoordinatorResourceTestClient; +import org.apache.druid.testing.clients.QueryResourceTestClient; +import org.apache.druid.testing.guice.DruidTestModuleFactory; +import org.apache.druid.testing.utils.ITRetryUtil; +import org.apache.druid.testing.utils.QueryResultVerifier; +import org.apache.druid.testing.utils.QueryWithResults; +import org.apache.druid.testing.utils.TestQueryHelper; +import org.apache.druid.tests.TestNGGroup; +import org.apache.druid.tests.indexer.AbstractIndexerTest; +import org.apache.druid.testsEx.config.BaseJUnitRule; +import org.jboss.netty.handler.codec.http.HttpResponseStatus; +import org.testng.Assert; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Guice; +import org.testng.annotations.Test; + +/** + * This class tests the query retry on missing segments. A segment can be missing in a historical during a query if + * the historical drops the segment after the broker issues the query to the historical. To mimic this case, this + * test spawns a historical modified for testing. This historical announces all segments assigned, but doesn't serve Review Comment: This is an example of a test that cannot be merged since it needs a custom cluster. ########## integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/query/ITSqlCancelTest.java: ########## @@ -0,0 +1,144 @@ +/* + * 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.druid.testsEx.query; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.collect.ImmutableMap; +import com.google.inject.Inject; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import org.apache.druid.java.util.common.ISE; +import org.apache.druid.java.util.common.RE; +import org.apache.druid.java.util.http.client.response.StatusResponseHolder; +import org.apache.druid.query.BaseQuery; +import org.apache.druid.query.QueryException; +import org.apache.druid.sql.http.SqlQuery; +import org.apache.druid.testing.IntegrationTestingConfig; +import org.apache.druid.testing.clients.SqlResourceTestClient; +import org.apache.druid.testing.utils.DataLoaderHelper; +import org.apache.druid.testing.utils.SqlTestQueryHelper; +import org.apache.druid.testsEx.categories.Query; +import org.apache.druid.testsEx.config.BaseJUnitRule; +import org.apache.druid.testsEx.config.DruidTestRunner; +import org.jboss.netty.handler.codec.http.HttpResponseStatus; +import org.junit.experimental.categories.Category; +import org.junit.runner.RunWith; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +@RunWith(DruidTestRunner.class) +@Category(Query.class) +public class ITSqlCancelTest extends BaseJUnitRule +{ + private static final String WIKIPEDIA_DATA_SOURCE = "wikipedia_editstream"; + + /** + * This query will run exactly for 15 seconds. + */ + private static final String QUERY + = "SELECT sleep(CASE WHEN added > 0 THEN 1 ELSE 0 END) FROM wikipedia_editstream WHERE added > 0 LIMIT 15"; Review Comment: This is a clever query: it avoids the `sleep` being done at plan time, which is what would happen if it were just `SELECT sleep(1) FROM wikipedia_editstream LIIT 15`. What are the arguments to `sleep`? A typical function takes a sleep time in seconds or milliseconds. Is our test function hard-coded to sleep for 15 seconds if we pass 1? Else, if the argument is a time in seconds, we're sleeping for 1 second, not 15. ########## integration-tests-ex/cases/cluster/Common/dependencies.yaml: ########## @@ -68,7 +68,7 @@ services: # The image will intialize the user and DB upon first start. metadata: # Uncomment the following when running on M1 Macs: - # platform: linux/x86_64 + platform: linux/x86_64 Review Comment: Did you want to check in this change? Is it safe to enable this for all platforms? If so, maybe change the comment to "Required for M1 Macs, harmless for other platforms." ########## integration-tests-ex/cases/src/test/java/org/apache/druid/testsEx/query/ITQueryErrorTest.java: ########## @@ -0,0 +1,246 @@ +/* + * 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.druid.testsEx.query; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.inject.Inject; +import java.io.IOException; +import java.util.HashMap; +import java.util.Map; +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.query.QueryContexts; +import org.apache.druid.server.coordination.ServerManagerForQueryErrorTest; +import org.apache.druid.testing.utils.DataLoaderHelper; +import org.apache.druid.testing.utils.SqlTestQueryHelper; +import org.apache.druid.testing.utils.TestQueryHelper; +import org.apache.druid.testsEx.categories.QueryError; +import org.apache.druid.testsEx.config.BaseJUnitRule; +import org.apache.druid.testsEx.indexer.AbstractIndexerTest; +import org.apache.druid.testsEx.config.DruidTestRunner; +import org.assertj.core.api.Assertions; +import org.junit.Before; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.runner.RunWith; + +/** + * This class tests various query failures. Review Comment: Each test category requires we start a cluster, run the test, and stop the cluster. At present, since we don't yet cache the Druid image, we also build the test image for every test. Given this, I wonder if we should combine the three query tests into a single category? We can do that by having three test files that are in the same category. This works as long as the tests are independent of one another. That is, test A does not create items on the Druid server that will throw off test B. If we do want to combine tests, it might make sense to first get them into GitHub as individual tests (to reduce risk), then follow up with another PR that does the combining. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
