[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6583: Implement QueryOp class.

[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