You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/02/16 19:43:29 UTC
[GitHub] [incubator-pinot] mqliang opened a new pull request #6583: Implement QueryOp class.
mqliang opened a new pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583
## Description
Implement QueryOp class, which take a file that has queries, and a file that has results, run the queries through the broker/server and compare the results with the result file.
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
* [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release.
If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text
## Documentation
If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r580704347
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +72,97 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
Review comment:
Why not use the `LOGGER` you specified in Line 41?
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +72,97 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ BufferedReader expectedResultReader = null;
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName), StandardCharsets.UTF_8))) {
+ if (_queryFileName == null) {
+ LOGGER.error("Result file is missing!");
+ return testPassed;
+ } else {
+ expectedResultReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8));
+ }
+
+ int passed = 0;
+ int total = 0;
+ int queryLineNum = 0;
+ String query;
+
+ while ((query = queryReader.readLine()) != null) {
+ queryLineNum++;
+ if (shouldIgnore(query)) {
+ continue;
+ }
+
+ JsonNode expectedJson = null;
+ try {
+ String expectedResultLine = expectedResultReader.readLine();
+ while (shouldIgnore(expectedResultLine)) {
+ expectedResultLine = expectedResultReader.readLine();
+ }
+ expectedJson = JsonUtils.stringToJsonNode(expectedResultLine);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while getting expected response for query: '{}'",
Review comment:
If an exception was caught when parsing the expected results, should we skip firing queries to the cluster and stop the comparison immediately?
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +72,97 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ BufferedReader expectedResultReader = null;
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName), StandardCharsets.UTF_8))) {
+ if (_queryFileName == null) {
+ LOGGER.error("Result file is missing!");
+ return testPassed;
+ } else {
+ expectedResultReader = new BufferedReader(
Review comment:
I think you can put this to the same try bracket in Line 89 as well, so that you don't need to specify the finally block at the end.
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/SegmentOp.java
##########
@@ -241,6 +242,27 @@ private boolean verifySegmentInState(String state)
return true;
}
+ private boolean verifyRoutingTableUpdated()
+ throws Exception {
+ String query = "SELECT count(*) FROM " + _tableName;
+ JsonNode result = QueryProcessor.postSqlQuery(query);
+ System.out.println(result);
Review comment:
remove this System.out.println()?
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -30,13 +38,22 @@
*/
@JsonIgnoreProperties(ignoreUnknown = true)
public class QueryOp extends BaseOp {
+ private static final Logger LOGGER = LoggerFactory.getLogger(QueryOp.class);
+
+ private static final String NUM_DOCS_SCANNED = "numDocsScanned";
+ private static final String TIME_USED_MS = "timeUsedMs";
private String _queryFileName;
private String _expectedResultsFileName;
public QueryOp() {
super(OpType.QUERY_OP);
}
+ private boolean shouldIgnore(String line) {
+ String trimmedLine = line.trim();
+ return trimmedLine.isEmpty() || trimmedLine.startsWith("#");
Review comment:
Put "#" into a constant.
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -30,13 +38,22 @@
*/
@JsonIgnoreProperties(ignoreUnknown = true)
public class QueryOp extends BaseOp {
+ private static final Logger LOGGER = LoggerFactory.getLogger(QueryOp.class);
+
+ private static final String NUM_DOCS_SCANNED = "numDocsScanned";
Review comment:
Rename it to `NUM_DOCS_SCANNED_KEY`. Same for the following one.
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +72,97 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ BufferedReader expectedResultReader = null;
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName), StandardCharsets.UTF_8))) {
+ if (_queryFileName == null) {
Review comment:
I'm not sure whether this if statement can ever be satisfied. Maybe we should check that before the try block?
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryProcessor.java
##########
@@ -0,0 +1,98 @@
+/**
+ * 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.pinot.compat.tests;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import java.io.BufferedReader;
+import java.io.BufferedWriter;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.io.OutputStreamWriter;
+import java.net.HttpURLConnection;
+import java.net.URL;
+import java.nio.charset.StandardCharsets;
+import java.util.Collections;
+import java.util.Map;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+public class QueryProcessor {
+
+ private static final String _brokerBaseApiUrl = ClusterDescriptor.CONTROLLER_URL;
Review comment:
Use snake case instead of camel case for static constant?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu merged pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mcvsubbu merged pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mqliang commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r582204971
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +72,97 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
Review comment:
Just wanner make it consistence with StreamOp and TableOp, basically every Op will print out such a info message in console.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#issuecomment-780110310
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=h1) Report
> Merging [#6583](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=desc) (c8d9ec0) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **increase** coverage by `7.18%`.
> The diff coverage is `79.12%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6583 +/- ##
==========================================
+ Coverage 66.44% 73.62% +7.18%
==========================================
Files 1075 1353 +278
Lines 54773 66619 +11846
Branches 8168 9716 +1548
==========================================
+ Hits 36396 49051 +12655
+ Misses 15700 14360 -1340
- Partials 2677 3208 +531
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.98% <39.73%> (?)` | |
| unittests | `65.55% <61.22%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `73.33% <ø> (+16.19%)` | :arrow_up: |
| [...n/java/org/apache/pinot/client/BrokerResponse.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Jyb2tlclJlc3BvbnNlLmphdmE=) | `100.00% <ø> (ø)` | |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `44.44% <ø> (-4.40%)` | :arrow_down: |
| [...n/java/org/apache/pinot/client/ExecutionStats.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0V4ZWN1dGlvblN0YXRzLmphdmE=) | `68.88% <ø> (ø)` | |
| [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `56.36% <ø> (-5.64%)` | :arrow_down: |
| [...n/java/org/apache/pinot/client/ResultSetGroup.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L1Jlc3VsdFNldEdyb3VwLmphdmE=) | `65.38% <ø> (+0.16%)` | :arrow_up: |
| [.../org/apache/pinot/client/ResultTableResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L1Jlc3VsdFRhYmxlUmVzdWx0U2V0LmphdmE=) | `88.00% <ø> (+53.71%)` | :arrow_up: |
| ... and [1188 more](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=footer). Last update [e4503a3...c8d9ec0](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r582220859
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +72,97 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
Review comment:
For now, it is ok. For some reason , logs don't show up when we run the ops via command-line. Eventually we will remove the sout or keep a small number of them if needed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r577971296
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/CompatibilityOpsRunner.java
##########
@@ -52,6 +51,11 @@ private boolean runOps()
System.out.println("Failure");
break;
}
+ /*
+ Brokers take time to update route table after a segment is created/uploaded/deleted, so sleep 1s between
+ two operations
+ */
+ TimeUnit.SECONDS.sleep(1);
Review comment:
Wait. I am not sure why the sleep or any kind of wait is needed here at all. Let us discuss.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mqliang commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r582223923
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +72,97 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ BufferedReader expectedResultReader = null;
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName), StandardCharsets.UTF_8))) {
+ if (_queryFileName == null) {
Review comment:
Use a try-with-resources statement now.
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -30,13 +38,22 @@
*/
@JsonIgnoreProperties(ignoreUnknown = true)
public class QueryOp extends BaseOp {
+ private static final Logger LOGGER = LoggerFactory.getLogger(QueryOp.class);
+
+ private static final String NUM_DOCS_SCANNED = "numDocsScanned";
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mqliang commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r583157392
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +73,87 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName), StandardCharsets.UTF_8));
+ BufferedReader expectedResultReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8))) {
+
+ int passed = 0;
Review comment:
done
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +73,87 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName), StandardCharsets.UTF_8));
+ BufferedReader expectedResultReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8))) {
+
+ int passed = 0;
+ int total = 0;
+ int queryLineNum = 0;
+ String query;
+
+ while ((query = queryReader.readLine()) != null) {
+ queryLineNum++;
+ if (shouldIgnore(query)) {
+ continue;
+ }
+
+ JsonNode expectedJson = null;
+ try {
+ String expectedResultLine = expectedResultReader.readLine();
+ while (shouldIgnore(expectedResultLine)) {
+ expectedResultLine = expectedResultReader.readLine();
+ }
+ expectedJson = JsonUtils.stringToJsonNode(expectedResultLine);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while getting expected response for query: '{}'",
+ queryLineNum, query, e);
+ }
+
+ JsonNode actualJson = null;
+ if (expectedJson != null) {
+ try {
+ actualJson = QueryProcessor.postSqlQuery(query);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while running query: '{}'", queryLineNum, query,
+ e);
+ }
+ }
+
+ if (expectedJson != null && actualJson != null) {
+ try {
+ boolean comparisonResult = SqlResultComparator.areEqual(actualJson, expectedJson, query);
Review comment:
done
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +73,87 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName), StandardCharsets.UTF_8));
+ BufferedReader expectedResultReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8))) {
+
+ int passed = 0;
+ int total = 0;
+ int queryLineNum = 0;
+ String query;
+
+ while ((query = queryReader.readLine()) != null) {
+ queryLineNum++;
+ if (shouldIgnore(query)) {
+ continue;
+ }
+
+ JsonNode expectedJson = null;
+ try {
+ String expectedResultLine = expectedResultReader.readLine();
+ while (shouldIgnore(expectedResultLine)) {
+ expectedResultLine = expectedResultReader.readLine();
+ }
+ expectedJson = JsonUtils.stringToJsonNode(expectedResultLine);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while getting expected response for query: '{}'",
+ queryLineNum, query, e);
+ }
+
+ JsonNode actualJson = null;
+ if (expectedJson != null) {
+ try {
+ actualJson = QueryProcessor.postSqlQuery(query);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while running query: '{}'", queryLineNum, query,
+ e);
+ }
+ }
+
+ if (expectedJson != null && actualJson != null) {
+ try {
+ boolean comparisonResult = SqlResultComparator.areEqual(actualJson, expectedJson, query);
+ if (comparisonResult) {
+ passed++;
+ LOGGER.info("Comparison PASSED: Line: {} actual Time: {} ms expected Time: {} ms Docs Scanned: {}",
Review comment:
done
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/SegmentOp.java
##########
@@ -67,8 +68,7 @@
private static final int DEFAULT_SLEEP_INTERVAL_MS = 200;
public enum Op {
- UPLOAD,
- DELETE
+ UPLOAD, DELETE
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r578115307
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/SegmentOp.java
##########
@@ -65,12 +66,6 @@
private static final FileFormat DEFAULT_FILE_FORMAT = FileFormat.CSV;
private static final int DEFAULT_MAX_SLEEP_TIME_MS = 30000;
private static final int DEFAULT_SLEEP_INTERVAL_MS = 200;
-
Review comment:
Why did you move this?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mqliang commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r582222757
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryProcessor.java
##########
@@ -0,0 +1,98 @@
+/**
+ * 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.pinot.compat.tests;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import java.io.BufferedReader;
+import java.io.BufferedWriter;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.io.OutputStreamWriter;
+import java.net.HttpURLConnection;
+import java.net.URL;
+import java.nio.charset.StandardCharsets;
+import java.util.Collections;
+import java.util.Map;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+public class QueryProcessor {
+
+ private static final String _brokerBaseApiUrl = ClusterDescriptor.CONTROLLER_URL;
Review comment:
done
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +72,97 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ BufferedReader expectedResultReader = null;
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName), StandardCharsets.UTF_8))) {
+ if (_queryFileName == null) {
+ LOGGER.error("Result file is missing!");
+ return testPassed;
+ } else {
+ expectedResultReader = new BufferedReader(
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mqliang commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r578116965
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/SegmentOp.java
##########
@@ -65,12 +66,6 @@
private static final FileFormat DEFAULT_FILE_FORMAT = FileFormat.CSV;
private static final int DEFAULT_MAX_SLEEP_TIME_MS = 30000;
private static final int DEFAULT_SLEEP_INTERVAL_MS = 200;
-
Review comment:
This is performed by IDEA reformat (after setting up the Pinot Code Style). I guess the original code does not reformat before committing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mqliang commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r577853189
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/CompatibilityOpsRunner.java
##########
@@ -52,6 +51,11 @@ private boolean runOps()
System.out.println("Failure");
break;
}
+ /*
+ Brokers take time to update route table after a segment is created/uploaded/deleted, so sleep 1s between
+ two operations
+ */
+ TimeUnit.SECONDS.sleep(1);
Review comment:
Both (1) and (2) need hardcode table name in code, which is not good in my opinion. Another idea is: in QueryOp.runOps() function, when test the first query, if the error message is "{errorCode: 410, message: "BrokerResourceMissingError"}", sleep and retry, until it succeeded or timeout.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#issuecomment-780110310
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=h1) Report
> Merging [#6583](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=desc) (c8d9ec0) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.45%`.
> The diff coverage is `39.73%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6583 +/- ##
===========================================
- Coverage 66.44% 43.98% -22.46%
===========================================
Files 1075 1353 +278
Lines 54773 66619 +11846
Branches 8168 9716 +1548
===========================================
- Hits 36396 29305 -7091
- Misses 15700 34856 +19156
+ Partials 2677 2458 -219
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.98% <39.73%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...t/broker/broker/BasicAuthAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0Jhc2ljQXV0aEFjY2Vzc0NvbnRyb2xGYWN0b3J5LmphdmE=) | `0.00% <0.00%> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
| ... and [1367 more](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=footer). Last update [e4503a3...c8d9ec0](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mqliang commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r582218992
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +72,97 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ BufferedReader expectedResultReader = null;
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName), StandardCharsets.UTF_8))) {
+ if (_queryFileName == null) {
+ LOGGER.error("Result file is missing!");
+ return testPassed;
+ } else {
+ expectedResultReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8));
+ }
+
+ int passed = 0;
+ int total = 0;
+ int queryLineNum = 0;
+ String query;
+
+ while ((query = queryReader.readLine()) != null) {
+ queryLineNum++;
+ if (shouldIgnore(query)) {
+ continue;
+ }
+
+ JsonNode expectedJson = null;
+ try {
+ String expectedResultLine = expectedResultReader.readLine();
+ while (shouldIgnore(expectedResultLine)) {
+ expectedResultLine = expectedResultReader.readLine();
+ }
+ expectedJson = JsonUtils.stringToJsonNode(expectedResultLine);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while getting expected response for query: '{}'",
Review comment:
From all-queries point of view, there is no early-termination logic here. Say we have 100 queries, if the first query failed, instead of returning test failure immediately, we will continue to verify the remaining 99 queries, and we will return test failure at the very end since 99 != 100. In this way, can detect as many problematic queries as possible in one go.
From a single query point of view, yes if an exception was caught when parsing the expected results, we do skip firing queries to the cluster:
```
if (expectedJson != null) {
try {
actualJson = QueryProcessor.postSqlQuery(query);
} catch (Exception e) {
LOGGER.error("Comparison FAILED: Line: {} Exception caught while running query: '{}'", queryLineNum, query,
e);
}
}
```
We only fire query to cluster if `expectedJson != null`
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/SegmentOp.java
##########
@@ -241,6 +242,27 @@ private boolean verifySegmentInState(String state)
return true;
}
+ private boolean verifyRoutingTableUpdated()
+ throws Exception {
+ String query = "SELECT count(*) FROM " + _tableName;
+ JsonNode result = QueryProcessor.postSqlQuery(query);
+ System.out.println(result);
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r583158060
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/CompatibilityOpsRunner.java
##########
@@ -36,8 +35,7 @@ private CompatibilityOpsRunner(String configFileName) {
_configFileName = configFileName;
}
- private boolean runOps()
- throws IOException, JsonParseException, JsonMappingException {
+ private boolean runOps() throws IOException, JsonParseException, JsonMappingException, InterruptedException {
Review comment:
I'd suggest throwing a generic `Exception` instead of putting all the concrete ones here.
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +72,97 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
Review comment:
Then why use `printf` instead of `println` here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r583221602
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +72,97 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
Review comment:
As suggested, please change this to println instead of printf. thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mqliang commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r583179637
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/CompatibilityOpsRunner.java
##########
@@ -36,8 +35,7 @@ private CompatibilityOpsRunner(String configFileName) {
_configFileName = configFileName;
}
- private boolean runOps()
- throws IOException, JsonParseException, JsonMappingException {
+ private boolean runOps() throws IOException, JsonParseException, JsonMappingException, InterruptedException {
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#issuecomment-784473648
Issue #4854
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mqliang commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r583157275
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/SegmentOp.java
##########
@@ -241,6 +242,26 @@ private boolean verifySegmentInState(String state)
return true;
}
+ private boolean verifyRoutingTableUpdated()
Review comment:
marked as TODO.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mqliang commented on pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mqliang commented on pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#issuecomment-780928422
I have address the comments:
1. reformat the code
2. add a "verifyRoutingTableUpdated" method in SegmentOp class instead of sleep between two Op
3. move send query related code to a dedicated QueryProcessor class, since SegmentOp also need to send query to verify routing table is updated.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r577304465
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/CompatibilityOpsRunner.java
##########
@@ -36,8 +36,7 @@ private CompatibilityOpsRunner(String configFileName) {
_configFileName = configFileName;
}
- private boolean runOps()
- throws IOException, JsonParseException, JsonMappingException {
+ private boolean runOps() throws IOException, JsonParseException, JsonMappingException, InterruptedException {
Review comment:
(nit) Please set up the Pinot Code Style following the instructions [here](https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup) and reformat the changes
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io commented on pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#issuecomment-780110310
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=h1) Report
> Merging [#6583](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=desc) (3a65467) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **increase** coverage by `7.14%`.
> The diff coverage is `79.40%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6583 +/- ##
==========================================
+ Coverage 66.44% 73.59% +7.14%
==========================================
Files 1075 1342 +267
Lines 54773 66021 +11248
Branches 8168 9630 +1462
==========================================
+ Hits 36396 48589 +12193
+ Misses 15700 14251 -1449
- Partials 2677 3181 +504
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.98% <40.66%> (?)` | |
| unittests | `65.41% <60.64%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `73.33% <ø> (+16.19%)` | :arrow_up: |
| [...n/java/org/apache/pinot/client/BrokerResponse.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Jyb2tlclJlc3BvbnNlLmphdmE=) | `100.00% <ø> (ø)` | |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `44.44% <ø> (-4.40%)` | :arrow_down: |
| [...org/apache/pinot/client/DynamicBrokerSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0R5bmFtaWNCcm9rZXJTZWxlY3Rvci5qYXZh) | `88.57% <ø> (+15.84%)` | :arrow_up: |
| [...n/java/org/apache/pinot/client/ExecutionStats.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0V4ZWN1dGlvblN0YXRzLmphdmE=) | `68.88% <ø> (ø)` | |
| [...va/org/apache/pinot/client/ExternalViewReader.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0V4dGVybmFsVmlld1JlYWRlci5qYXZh) | `81.69% <ø> (+31.69%)` | :arrow_up: |
| [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `56.36% <ø> (-5.64%)` | :arrow_down: |
| ... and [1161 more](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=footer). Last update [e4503a3...1464a8b](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#issuecomment-780110310
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=h1) Report
> Merging [#6583](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=desc) (a73b47d) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.60%`.
> The diff coverage is `42.75%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6583 +/- ##
===========================================
- Coverage 66.44% 43.84% -22.61%
===========================================
Files 1075 1353 +278
Lines 54773 66619 +11846
Branches 8168 9716 +1548
===========================================
- Hits 36396 29210 -7186
- Misses 15700 34952 +19252
+ Partials 2677 2457 -220
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.84% <42.75%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...t/broker/broker/BasicAuthAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0Jhc2ljQXV0aEFjY2Vzc0NvbnRyb2xGYWN0b3J5LmphdmE=) | `0.00% <0.00%> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
| ... and [1357 more](https://codecov.io/gh/apache/incubator-pinot/pull/6583/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=footer). Last update [e4503a3...c8d9ec0](https://codecov.io/gh/apache/incubator-pinot/pull/6583?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r583133785
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +73,87 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName), StandardCharsets.UTF_8));
+ BufferedReader expectedResultReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8))) {
+
+ int passed = 0;
+ int total = 0;
+ int queryLineNum = 0;
+ String query;
+
+ while ((query = queryReader.readLine()) != null) {
+ queryLineNum++;
+ if (shouldIgnore(query)) {
+ continue;
+ }
+
+ JsonNode expectedJson = null;
+ try {
+ String expectedResultLine = expectedResultReader.readLine();
+ while (shouldIgnore(expectedResultLine)) {
+ expectedResultLine = expectedResultReader.readLine();
+ }
+ expectedJson = JsonUtils.stringToJsonNode(expectedResultLine);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while getting expected response for query: '{}'",
+ queryLineNum, query, e);
+ }
+
+ JsonNode actualJson = null;
+ if (expectedJson != null) {
+ try {
+ actualJson = QueryProcessor.postSqlQuery(query);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while running query: '{}'", queryLineNum, query,
+ e);
+ }
+ }
+
+ if (expectedJson != null && actualJson != null) {
+ try {
+ boolean comparisonResult = SqlResultComparator.areEqual(actualJson, expectedJson, query);
Review comment:
```suggestion
boolean passed = SqlResultComparator.areEqual(actualJson, expectedJson, query);
```
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +73,87 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName), StandardCharsets.UTF_8));
+ BufferedReader expectedResultReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8))) {
+
+ int passed = 0;
+ int total = 0;
Review comment:
```suggestion
int totalQueryCount = 0;
```
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/SegmentOp.java
##########
@@ -67,8 +68,7 @@
private static final int DEFAULT_SLEEP_INTERVAL_MS = 200;
public enum Op {
- UPLOAD,
- DELETE
+ UPLOAD, DELETE
Review comment:
Keep them in separate lines please
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +73,87 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName), StandardCharsets.UTF_8));
+ BufferedReader expectedResultReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8))) {
+
+ int passed = 0;
+ int total = 0;
+ int queryLineNum = 0;
+ String query;
+
+ while ((query = queryReader.readLine()) != null) {
+ queryLineNum++;
+ if (shouldIgnore(query)) {
+ continue;
+ }
+
+ JsonNode expectedJson = null;
+ try {
+ String expectedResultLine = expectedResultReader.readLine();
+ while (shouldIgnore(expectedResultLine)) {
+ expectedResultLine = expectedResultReader.readLine();
+ }
+ expectedJson = JsonUtils.stringToJsonNode(expectedResultLine);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while getting expected response for query: '{}'",
+ queryLineNum, query, e);
+ }
+
+ JsonNode actualJson = null;
+ if (expectedJson != null) {
+ try {
+ actualJson = QueryProcessor.postSqlQuery(query);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while running query: '{}'", queryLineNum, query,
+ e);
+ }
+ }
+
+ if (expectedJson != null && actualJson != null) {
+ try {
+ boolean comparisonResult = SqlResultComparator.areEqual(actualJson, expectedJson, query);
+ if (comparisonResult) {
+ passed++;
+ LOGGER.info("Comparison PASSED: Line: {} actual Time: {} ms expected Time: {} ms Docs Scanned: {}",
Review comment:
We should not be logging expected time, etc. We should not even have it in metadata. We can add some outliers for time later. For now, remove it from the results file and also from this log.
Also, we don't need an INFO log for every passing query. Just add a single debug log with query, expected result and actual result
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +73,87 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName), StandardCharsets.UTF_8));
+ BufferedReader expectedResultReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8))) {
+
+ int passed = 0;
Review comment:
```suggestion
int SucceededQueryCount = 0;
```
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/SegmentOp.java
##########
@@ -241,6 +242,26 @@ private boolean verifySegmentInState(String state)
return true;
}
+ private boolean verifyRoutingTableUpdated()
Review comment:
You should really be getting the number of rows before adding the segment, and then the number of rows after adding the segment and make sure that it has increased by the number of rows in the segment. If you do this, then you can remove the other check of segment state model.online.
You can mark this as TODO and take it on in a separate PR
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +73,87 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName), StandardCharsets.UTF_8));
+ BufferedReader expectedResultReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8))) {
+
+ int passed = 0;
+ int total = 0;
+ int queryLineNum = 0;
+ String query;
+
+ while ((query = queryReader.readLine()) != null) {
+ queryLineNum++;
+ if (shouldIgnore(query)) {
+ continue;
+ }
+
+ JsonNode expectedJson = null;
+ try {
+ String expectedResultLine = expectedResultReader.readLine();
+ while (shouldIgnore(expectedResultLine)) {
+ expectedResultLine = expectedResultReader.readLine();
+ }
+ expectedJson = JsonUtils.stringToJsonNode(expectedResultLine);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while getting expected response for query: '{}'",
+ queryLineNum, query, e);
+ }
+
+ JsonNode actualJson = null;
+ if (expectedJson != null) {
+ try {
+ actualJson = QueryProcessor.postSqlQuery(query);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while running query: '{}'", queryLineNum, query,
+ e);
+ }
+ }
+
+ if (expectedJson != null && actualJson != null) {
+ try {
+ boolean comparisonResult = SqlResultComparator.areEqual(actualJson, expectedJson, query);
+ if (comparisonResult) {
+ passed++;
+ LOGGER.info("Comparison PASSED: Line: {} actual Time: {} ms expected Time: {} ms Docs Scanned: {}",
+ queryLineNum, actualJson.get(TIME_USED_MS_KEY), expectedJson.get(TIME_USED_MS_KEY),
+ actualJson.get(NUM_DOCS_SCANNED_KEY));
+ LOGGER.debug("actual Response: {}", actualJson);
Review comment:
Please combine these two logs into one log.
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +73,87 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName), StandardCharsets.UTF_8));
+ BufferedReader expectedResultReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8))) {
+
+ int passed = 0;
+ int total = 0;
+ int queryLineNum = 0;
+ String query;
+
+ while ((query = queryReader.readLine()) != null) {
+ queryLineNum++;
+ if (shouldIgnore(query)) {
+ continue;
+ }
+
+ JsonNode expectedJson = null;
+ try {
+ String expectedResultLine = expectedResultReader.readLine();
+ while (shouldIgnore(expectedResultLine)) {
+ expectedResultLine = expectedResultReader.readLine();
+ }
+ expectedJson = JsonUtils.stringToJsonNode(expectedResultLine);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while getting expected response for query: '{}'",
+ queryLineNum, query, e);
+ }
+
+ JsonNode actualJson = null;
+ if (expectedJson != null) {
+ try {
+ actualJson = QueryProcessor.postSqlQuery(query);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while running query: '{}'", queryLineNum, query,
+ e);
+ }
+ }
+
+ if (expectedJson != null && actualJson != null) {
+ try {
+ boolean comparisonResult = SqlResultComparator.areEqual(actualJson, expectedJson, query);
+ if (comparisonResult) {
+ passed++;
+ LOGGER.info("Comparison PASSED: Line: {} actual Time: {} ms expected Time: {} ms Docs Scanned: {}",
+ queryLineNum, actualJson.get(TIME_USED_MS_KEY), expectedJson.get(TIME_USED_MS_KEY),
+ actualJson.get(NUM_DOCS_SCANNED_KEY));
+ LOGGER.debug("actual Response: {}", actualJson);
+ LOGGER.debug("expected Response: {}", expectedJson);
+ } else {
+ LOGGER.error("Comparison FAILED: Line: {} query: {}", queryLineNum, query);
Review comment:
Single log.error will suffice.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mqliang commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mqliang commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r582218992
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -55,7 +72,97 @@ public void setExpectedResultsFileName(String expectedResultsFileName) {
@Override
boolean runOp() {
- System.out.println("Verifying queries in " + _queryFileName + " against results in " + _expectedResultsFileName);
- return true;
+ System.out.printf("Verifying queries in %s against results in %s\n", _queryFileName, _expectedResultsFileName);
+ try {
+ return verifyQueries();
+ } catch (Exception e) {
+ LOGGER.error("FAILED to verify queries in {}: {}", _queryFileName, e);
+ return false;
+ }
+ }
+
+ boolean verifyQueries()
+ throws Exception {
+ BufferedReader expectedResultReader = null;
+ boolean testPassed = false;
+
+ try (BufferedReader queryReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_queryFileName), StandardCharsets.UTF_8))) {
+ if (_queryFileName == null) {
+ LOGGER.error("Result file is missing!");
+ return testPassed;
+ } else {
+ expectedResultReader = new BufferedReader(
+ new InputStreamReader(new FileInputStream(_expectedResultsFileName), StandardCharsets.UTF_8));
+ }
+
+ int passed = 0;
+ int total = 0;
+ int queryLineNum = 0;
+ String query;
+
+ while ((query = queryReader.readLine()) != null) {
+ queryLineNum++;
+ if (shouldIgnore(query)) {
+ continue;
+ }
+
+ JsonNode expectedJson = null;
+ try {
+ String expectedResultLine = expectedResultReader.readLine();
+ while (shouldIgnore(expectedResultLine)) {
+ expectedResultLine = expectedResultReader.readLine();
+ }
+ expectedJson = JsonUtils.stringToJsonNode(expectedResultLine);
+ } catch (Exception e) {
+ LOGGER.error("Comparison FAILED: Line: {} Exception caught while getting expected response for query: '{}'",
Review comment:
From all-queries point of view, there is no early-termination logic here. Say we have 100 queries, if the first query failed, instead of returning test failure immediately, we will continue to verify the remaining 99 queries, and we will return test failure at the very end since 99 != 100. In this way, can detect as many problematic queries as possible in one go.
From a single query point of view, yes if an exception was caught when parsing the expected results, we indeed skip firing queries to the cluster:
```
if (expectedJson != null) {
try {
actualJson = QueryProcessor.postSqlQuery(query);
} catch (Exception e) {
LOGGER.error("Comparison FAILED: Line: {} Exception caught while running query: '{}'", queryLineNum, query,
e);
}
}
```
We only fire query to cluster if `expectedJson != null`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mqliang commented on pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mqliang commented on pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#issuecomment-780087492
@mcvsubbu
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6583: Implement QueryOp class.
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6583:
URL: https://github.com/apache/incubator-pinot/pull/6583#discussion_r577777777
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/CompatibilityOpsRunner.java
##########
@@ -36,8 +36,7 @@ private CompatibilityOpsRunner(String configFileName) {
_configFileName = configFileName;
}
- private boolean runOps()
- throws IOException, JsonParseException, JsonMappingException {
+ private boolean runOps() throws IOException, JsonParseException, JsonMappingException, InterruptedException {
Review comment:
+1
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/CompatibilityOpsRunner.java
##########
@@ -52,6 +51,11 @@ private boolean runOps()
System.out.println("Failure");
break;
}
+ /*
+ Brokers take time to update route table after a segment is created/uploaded/deleted, so sleep 1s between
+ two operations
+ */
+ TimeUnit.SECONDS.sleep(1);
Review comment:
Adding a sleep here can make tests unreliable and susceptible to intermittent failures. You should add logic like the following:
`
ready = test_broker();
while (!ready) {
sleep(200ms)
ready = test_broker();
}
`
In test_broker(), you can do one of:
(1) make a call to the debug port of the broker to ensure that routing tables are built
(2) send a query to verify
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -30,13 +49,34 @@
*/
@JsonIgnoreProperties(ignoreUnknown = true)
public class QueryOp extends BaseOp {
+ private static final Logger LOGGER = LoggerFactory.getLogger(QueryOp.class);
+
+ private static final String NUM_DOCS_SCANNED = "numDocsScanned";
+ private static final String TIME_USED_MS = "timeUsedMs";
+ private final String _brokerBaseApiUrl = ClusterDescriptor.CONTROLLER_URL;
private String _queryFileName;
private String _expectedResultsFileName;
public QueryOp() {
super(OpType.QUERY_OP);
}
+ private static boolean shouldIgnore(String line) {
+ String trimmedLine = line.trim();
+ return trimmedLine.isEmpty() || trimmedLine.startsWith("#");
+ }
+
+ private static String constructResponse(InputStream inputStream) throws IOException {
Review comment:
why static?
##########
File path: pinot-integration-tests/src/test/java/org/apache/pinot/compat/tests/QueryOp.java
##########
@@ -30,13 +49,34 @@
*/
@JsonIgnoreProperties(ignoreUnknown = true)
public class QueryOp extends BaseOp {
+ private static final Logger LOGGER = LoggerFactory.getLogger(QueryOp.class);
+
+ private static final String NUM_DOCS_SCANNED = "numDocsScanned";
+ private static final String TIME_USED_MS = "timeUsedMs";
+ private final String _brokerBaseApiUrl = ClusterDescriptor.CONTROLLER_URL;
private String _queryFileName;
private String _expectedResultsFileName;
public QueryOp() {
super(OpType.QUERY_OP);
}
+ private static boolean shouldIgnore(String line) {
Review comment:
why static?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org