[GitHub] [gobblin] umustafi commented on a diff in pull request #3533: [GOBBLIN-1675] Add pagination for GaaS on server side

[GitBox <gi...@apache.org>]

umustafi commented on code in PR #3533:
URL: https://github.com/apache/gobblin/pull/3533#discussion_r940664321


##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/FlowConfigsV2Resource.java:
##########
@@ -125,9 +132,18 @@ public List<FlowConfig> getFilteredFlows(@Context PagingContext context,
       @Optional @QueryParam("isRunImmediately") Boolean isRunImmediately,
       @Optional @QueryParam("owningGroup") String owningGroup,
       @Optional @QueryParam("propertyFilter") String propertyFilter) {
-    FlowSpecSearchObject flowSpecSearchObject = new FlowSpecSearchObject(null, flowGroup, flowName,
-        templateUri, userToProxy, sourceIdentifier, destinationIdentifier, schedule, null,
-        isRunImmediately, owningGroup, propertyFilter);
+    FlowSpecSearchObject flowSpecSearchObject;
+    if (!context.hasCount() && !context.hasStart()){
+      flowSpecSearchObject = new FlowSpecSearchObject(null, flowGroup, flowName,
+          templateUri, userToProxy, sourceIdentifier, destinationIdentifier, schedule, null,
+          isRunImmediately, owningGroup, propertyFilter, -1, -1, null);
+    }
+    else {
+      flowSpecSearchObject = new FlowSpecSearchObject(null, flowGroup, flowName,
+          templateUri, userToProxy, sourceIdentifier, destinationIdentifier, schedule, null,
+          isRunImmediately, owningGroup, propertyFilter, context.getStart(), context.getCount(), null);

Review Comment:
   what do the start and count values represent? 



##########
gobblin-runtime/src/test/java/org/apache/gobblin/runtime/spec_store/MysqlSpecStoreTest.java:
##########
@@ -240,6 +243,40 @@ public void testGetSpecWithTag() throws Exception {
     Assert.assertEquals(result.size(), 2);
   }
 
+  @Test (dependsOnMethods = "testGetSpec")
+  public void testGetFilterSpecPaginate() throws Exception {
+    FlowSpecSearchObject flowSpecSearchObject = FlowSpecSearchObject.builder().count(1).start(0).propertyFilter("filter.this.flow").build();
+    Collection<Spec> specs = this.specStore.getSpecs(flowSpecSearchObject);
+    Assert.assertEquals(specs.size(), 1);
+    Assert.assertTrue(specs.contains(this.flowSpec1));
+    Assert.assertFalse(specs.contains(this.flowSpec2));
+    Assert.assertFalse(specs.contains(this.flowSpec4));
+
+    flowSpecSearchObject = FlowSpecSearchObject.builder().count(1).start(1).propertyFilter("filter.this.flow").build();
+    specs = this.specStore.getSpecs(flowSpecSearchObject);
+    Assert.assertEquals(specs.size(), 1);
+    Assert.assertFalse(specs.contains(this.flowSpec1));
+    Assert.assertTrue(specs.contains(this.flowSpec2));
+    Assert.assertFalse(specs.contains(this.flowSpec4));
+
+    flowSpecSearchObject = FlowSpecSearchObject.builder().count(5).start(0).propertyFilter("filter.this.flow").build();
+    specs = this.specStore.getSpecs(flowSpecSearchObject);

Review Comment:
   its not very clear to me how the count and start values are changing the spec catalog size here. Is count the limit to number it adds? Is start the offset from the start of the flow catalog? It would be helpful to add a comment here to explain why a certain flow spec should not be contained here. ie: the start value appears to be changing which flow specs are added in the top two cases so you should make that clear. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org