amogh-jahagirdar commented on code in PR #13400:
URL: https://github.com/apache/iceberg/pull/13400#discussion_r2538882579
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -247,6 +273,57 @@ public static <T> T roundTripSerialize(T payload, String
description) {
return null;
}
+ /** Configurable planning behavior for tests. Replaces magic table
name-based behavior routing. */
+ static class TestPlanningBehavior implements
RESTCatalogAdapter.PlanningBehavior {
+ private boolean asyncPlanning;
+ private int tasksPerPage;
+
+ static Builder builder() {
+ return new Builder();
+ }
+
+ @Override
+ public boolean shouldPlanTableScanAsync(TableScan tableScan) {
+ return asyncPlanning;
+ }
+
+ @Override
+ public int numberFileScanTasksPerPlanTask() {
+ return tasksPerPage;
+ }
+
+ static class Builder {
+ private TestPlanningBehavior behavior = new TestPlanningBehavior();
Review Comment:
I feel like we're overcomplicating this quite a bit. Are we not able to just
inline override RestCatalogAdapter just once for all these tests, and then just
setup the tests on the basis of how the behavior is overriden? I don't think we
need to set this per test in different ways.
Also, to make sure we are indeed exercising the paths we expect, it'd be
nice to verify the actual http calls made if that's reasonable.
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -247,6 +273,57 @@ public static <T> T roundTripSerialize(T payload, String
description) {
return null;
}
+ /** Configurable planning behavior for tests. Replaces magic table
name-based behavior routing. */
+ static class TestPlanningBehavior implements
RESTCatalogAdapter.PlanningBehavior {
+ private boolean asyncPlanning;
+ private int tasksPerPage;
+
+ static Builder builder() {
+ return new Builder();
+ }
+
+ @Override
+ public boolean shouldPlanTableScanAsync(TableScan tableScan) {
+ return asyncPlanning;
+ }
+
+ @Override
+ public int numberFileScanTasksPerPlanTask() {
+ return tasksPerPage;
+ }
+
+ static class Builder {
+ private TestPlanningBehavior behavior = new TestPlanningBehavior();
Review Comment:
I feel like we're overcomplicating this quite a bit. Are we not able to just
inline override the behavior in the adapter just once for all these tests, and
then just setup the tests on the basis of how the behavior is overriden? I
don't think we need to set this per test in different ways.
Also, to make sure we are indeed exercising the paths we expect, it'd be
nice to verify the actual http calls made if that's reasonable.
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -247,6 +273,57 @@ public static <T> T roundTripSerialize(T payload, String
description) {
return null;
}
+ /** Configurable planning behavior for tests. Replaces magic table
name-based behavior routing. */
+ static class TestPlanningBehavior implements
RESTCatalogAdapter.PlanningBehavior {
+ private boolean asyncPlanning;
+ private int tasksPerPage;
+
+ static Builder builder() {
+ return new Builder();
+ }
+
+ @Override
+ public boolean shouldPlanTableScanAsync(TableScan tableScan) {
+ return asyncPlanning;
+ }
+
+ @Override
+ public int numberFileScanTasksPerPlanTask() {
+ return tasksPerPage;
+ }
+
+ static class Builder {
+ private TestPlanningBehavior behavior = new TestPlanningBehavior();
Review Comment:
I feel like we're overcomplicating this quite a bit. Are we not able to just
inline override the behavior in the adapter just once for all these tests, and
then just setup the tests on the basis of how the behavior is overriden? I
don't think we need to set this _per_ test in different ways.
Also, to make sure we are indeed exercising the paths we expect, it'd be
nice to verify the actual http calls made if that's reasonable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
