>From Ian Maxon <[email protected]>: Attention is currently required from: Hongyu Shi.
Ian Maxon has posted comments on this change by Hongyu Shi. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744?usp=email ) Change subject: [ASTERIXDB-3681]Workload manager with configurable priority-based scheduling ...................................................................... Patch Set 3: (24 comments) Patchset: PS3: these are just some general questions/comments, i still need to examine all the actual scheduling and metadata stuff File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/QueryResultApiServlet.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/6eab11cd_12fab7a8?usp=email : PS3, Line 249: revert File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/result/fields/MetricsPrinter.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/f16dd0eb_37fbd0eb?usp=email : PS3, Line 51: DDED_TO_THE_MEMORY_QUEUE_TIME_NANO("addedToTheMemoryQueueTimeNano"), it's not clear to me what the difference is between this and the non-memory field. is there some other way to name these perhaps that clarifies it? File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/91a0c869_02581087?usp=email : PS3, Line 25: SchedulerConfigMetadataEntity the linter will freak out if you use star imports (and they're generally bad) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/d640d098_cde0e437?usp=email : PS3, Line 6582: return ccs.getJobManager() instanceof WorkloadManager; adding a method that returns an enum or something depending on whether or not the implementation supports workload scheduling is much better than instanceof File asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/CCApplication.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/350962a5_52475059?usp=email : PS3, Line 492: cc.getExecutor().submit(() -> { this can't be done like this. why not use something like IClusterStateManager::waitForState at the bare minimum? File asterixdb/asterix-app/src/test/java/org/apache/asterix/api/common/AsterixHyracksIntegrationUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/0aa6a806_a495a26c?usp=email : PS3, Line 485: if (cc.getJobManager() instanceof WorkloadManager) { no instanceof here, and if you have that method that hides the enum check or whatever, make it a static final method so it can be reused every time you have to gate on the scheduler being enabled or not File asterixdb/asterix-app/src/test/resources/cc-scheduler.conf: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/10c98823_f7bad472?usp=email : PS3, Line 51: job.manager.class=org.apache.hyracks.control.cc.job.WorkloadManager i think this should just be a boolean, not a class. the 'app.class' config var makes sense because Hyracks presents an API that is intended to be extended by various applications, but that isn't true for the workload manager. it's not like there will be possibly many workload manager implementations the user would need to be able to switch between. File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/util/SchedulerConfigUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/ec16f54e_432ae6b5?usp=email : PS3, Line 36: //--------------------------------------- Scheduler config --------------------------------------// : : /* : Example of full-text config create statement : CREATE SCHEDULER CONFIG s_config_1 { : "defaultPriority": 1, : "shortMemoryPercent": 10.0, : "shortCPUQuota": 20, : "queryGroup": : [ : { : "priority": 4, : "grouplist": [ "ui"] : }, : : { : "priority" : 6, : "grouplist": ["analytics"] : : }, : : { : "priority": 8, : "grouplist": [ "management" ] : } : ] : }; : : UPSERT QGROUP INTO s_config_1 {[ : { : "priority": 10, : "name": "ingest" : }, : : { : "priority": 6, : "name": "management" : } : ]}; : : DELETE GROUP FROM s_config_1 {[ : "analytics", "ingest" : ]}; : : delete this comment File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/MetadataTransactionContext.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/a457daf6_0c2ea1ff?usp=email : PS3, Line 178: public void addSchedulerConfig(SchedulerConfigMetadataEntity configMetadataEntity) { : // TODO : } is this still todo? File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/scheduler/SchedulerConfigRecordDescriptor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/885db292_4d999b9d?usp=email : PS3, Line 39: //TODO(DB): database name should not be null ? File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/ClusterControllerService.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/412a291b_ab2b7e91?usp=email : PS3, Line 258: /* jobManager = new WorkloadManager(ccConfig, this, jobCapacityController); */ ? File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/job/JobRun.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/2bbf4f93_9c86617d?usp=email : PS3, Line 445: public ObjectNode toJSON_Shortened() { : ObjectMapper om = new ObjectMapper(); : ObjectNode result = om.createObjectNode(); : : result.put("job-id", jobId.toString()); : result.putPOJO("status", getStatus()); : result.put("create-time", getCreateTime()); : result.put("start-time", getStartTime()); : result.put("end-time", getEndTime()); : // if (getJobSpecification().getSizeTag() != null) { : // result.put("query-size", getJobSpecification().getSizeTag().toString()); : // } : long executionTime = getEndTime() - getStartTime(); : long waitTime = getStartTime() - getCreateTime(); : result.put("slow-down", (double) (waitTime + executionTime) / executionTime); : return result; : } : is this still needed? or was this for benchmarking? File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/job/WorkloadManager.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/b0e787fb_1e9eb32e?usp=email : PS3, Line 49: WorkloadManager what made you decide to put this as a new class that extends jobmanager instead of just adding it directly? it seems kind of weird at some points. for example a lot of the code in finalComplete has to be duplicated because currently that's where a lot of the basic capacity management lies. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/aa615ba2_d065966f?usp=email : PS3, Line 170: logJobCapacity(jobRun, "released", Level.DEBUG); might want this at TRACE File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/scheduler/CapacityControllerGuard.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/afc57dd1_b8a898cb?usp=email : PS3, Line 208: /* TODO: dynamically adjust resources limits from different categories */ : /* CPU Quota : short common : limit: 6 6 : used: 4 0 : available: 2 6 : -> set short to 3 : since 2 < 6 - 3 = 3 : limit: 3 9 : used: 4 0 : available: 0 8 : -> four used CPU Quota for short job is released : available: : 3 9 : */ remove this comment? File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/scheduler/CompositeQueue.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/3432700f_1497db02?usp=email : PS3, Line 38: //private final IJobCapacityController jobCapacityController; ? File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/scheduler/DedicatedJobQueue.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/683a5f96_93850cc3?usp=email : PS3, Line 89: // Removes the selected job. remove https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/492d070b_3e0124a8?usp=email : PS3, Line 90: /* TODO: More bookkeeping for short jobs*/ what would this bookkeeping be? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/7c9d816f_2dc449e2?usp=email : PS3, Line 100: / Fails the job. remove File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/scheduler/DefaultJobQueue.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/00e50331_8ac5b3f9?usp=email : PS3, Line 51: //private final Map<JobId, JobRun> memoryQueue = new LinkedHashMap<>(); ? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/0459647a_d3269648?usp=email : PS3, Line 54: //private final IJobCapacityController jobCapacityController; ? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/b9b36213_578de3e1?usp=email : PS3, Line 65: double[] candidateExecTimes = : new double[] { 337.32, 0.5, 5.5, 27.33, 41.4, 58.55, 76.27, 124.3, 160.46, 215.23, 295.49 }; what are these numbers? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744/comment/2ef9fd08_ecfe000b?usp=email : PS3, Line 140: if (ratio <= 0.05) { : return queues.get(1); : } else if (ratio <= 0.10) { : return queues.get(2); : } else if (ratio <= 0.15) { : return queues.get(3); : } else if (ratio <= 0.20) { : return queues.get(4); : } else if (ratio <= 0.25) { : return queues.get(5); : } else if (ratio <= 0.40) { : return queues.get(6); : } else if (ratio <= 0.55) { : return queues.get(7); : } else if (ratio <= 0.70) { : return queues.get(8); : } else if (ratio <= 0.85) { : return queues.get(9); : } : return queues.get(10); maybe make this a switch/case? -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20744?usp=email To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: I52748f8e62c29e3595898f765d00fd33320316c3 Gerrit-Change-Number: 20744 Gerrit-PatchSet: 3 Gerrit-Owner: Hongyu Shi <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-CC: Ian Maxon <[email protected]> Gerrit-Attention: Hongyu Shi <[email protected]> Gerrit-Comment-Date: Wed, 07 Jan 2026 00:10:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No
