Xiao-zhen-Liu commented on code in PR #4205:
URL: https://github.com/apache/texera/pull/4205#discussion_r2794829244
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/controller/promisehandlers/QueryWorkerStatisticsHandler.scala:
##########
Review Comment:
Please also update this comment as it is not just the frontend.
##########
common/config/src/main/resources/application.conf:
##########
@@ -31,6 +31,9 @@ constants {
status-update-interval = 500
status-update-interval = ${?CONSTANTS_STATUS_UPDATE_INTERVAL}
+
+ runtime-statistics-interval = 2000
Review Comment:
It should be more explicit that this config is about persistence. Please add
it in the name.
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/controller/ControllerTimerService.scala:
##########
@@ -36,28 +37,61 @@ class ControllerTimerService(
akkaActorService: AkkaActorService
) {
var statusUpdateAskHandle: Option[Cancellable] = None
+ var runtimeStatisticsAskHandle: Option[Cancellable] = None
- def enableStatusUpdate(): Unit = {
- if (controllerConfig.statusUpdateIntervalMs.nonEmpty &&
statusUpdateAskHandle.isEmpty) {
- statusUpdateAskHandle = Option(
+ private def enableTimer(
+ intervalMs: Option[Long],
+ updateTarget: StatisticsUpdateTarget,
+ handleOpt: Option[Cancellable]
+ ): Option[Cancellable] = {
+ if (intervalMs.nonEmpty && handleOpt.isEmpty) {
+ Option(
akkaActorService.sendToSelfWithFixedDelay(
0.milliseconds,
- FiniteDuration.apply(controllerConfig.statusUpdateIntervalMs.get,
MILLISECONDS),
+ FiniteDuration.apply(intervalMs.get, MILLISECONDS),
ControlInvocation(
METHOD_CONTROLLER_INITIATE_QUERY_STATISTICS,
- QueryStatisticsRequest(Seq.empty),
+ QueryStatisticsRequest(Seq.empty, updateTarget),
Review Comment:
As you are having two separate timers that each send separate
`QueryStatisticsRequest`s, more requests will be sent than before. I am
wondering what would be the implication of this? For example, would more
frequent `QueryStatistics` be sent to each worker? It would be good if you can
comment on this in your PR description.
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/controller/promisehandlers/QueryWorkerStatisticsHandler.scala:
##########
Review Comment:
The default configs of 500ms vs. 2000ms might result in each
`RuntimeStatisticsPersist` coinciding with `ExecutionStatsUpdate`. Will all the
`RuntimeStatisticsPersist` requests be discarded?
--
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]
