aglinxinyuan commented on code in PR #6797:
URL: https://github.com/apache/texera/pull/6797#discussion_r3635637766
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/common/PekkoActorRefMappingService.scala:
##########
@@ -76,13 +76,13 @@ class PekkoActorRefMappingService(actorService:
PekkoActorService) extends Amber
def removeActorRef(id: ActorVirtualIdentity): Unit = {
if (actorRefMapping.contains(id)) {
val ref = actorRefMapping.remove(id).get
- logger.warn(s"actor $id is not reachable anymore, it might have crashed.
old ref = $ref")
+ logger.debug(s"actor $id is not reachable anymore, it might have
crashed. old ref = $ref")
}
Review Comment:
Agreed — reworded in 6812804910 to a neutral `removed actor ref for <id>.
old ref = <ref>`, since `removeActorRef` also runs during normal graceful
region teardown, so the old "might have crashed" wording was misleading even at
DEBUG.
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/worker/DataProcessor.scala:
##########
@@ -242,14 +242,14 @@ class DataProcessor(
): Unit = {
inputManager.currentChannelId = channelId
val command = ecm.commandMapping.get(actorId.name)
- logger.info(s"receive ECM from $channelId, id = ${ecm.id}, cmd = $command")
+ logger.debug(s"receive ECM from $channelId, id = ${ecm.id}, cmd =
$command")
if (ecm.ecmType != NO_ALIGNMENT) {
pauseManager.pauseInputChannel(ECMPause(ecm.id), List(channelId))
}
if (ecmManager.isECMAligned(channelId, ecm)) {
logManager.markAsReplayDestination(ecm.id)
// invoke the control command carried with the ECM
- logger.info(s"process ECM from $channelId, id = ${ecm.id}, cmd =
$command")
+ logger.debug(s"process ECM from $channelId, id = ${ecm.id}, cmd =
$command")
Review Comment:
Not needed here — these go through `com.typesafe.scala-logging` 3.9.6, whose
`Logger.debug`/`warn`/… methods are macros that expand to `if
(underlying.isDebugEnabled) underlying.debug(…)`. The interpolated `s"…"` (and
the `+ other` concat) is part of that guarded argument, so it isn't evaluated
when the level is disabled. Moving to SLF4J `{}` placeholders would be
non-idiomatic for this codebase and gains nothing, so I've left these as-is.
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/worker/DataProcessor.scala:
##########
@@ -268,7 +268,7 @@ class DataProcessor(
outputManager.flush(Some(downstreamChannelsInScope))
outputGateway.getActiveChannels.foreach { activeChannelId =>
if (downstreamChannelsInScope.contains(activeChannelId)) {
- logger.info(
+ logger.debug(
s"send ECM to $activeChannelId, id = ${ecm.id}, cmd = $command"
)
Review Comment:
Not needed here — these go through `com.typesafe.scala-logging` 3.9.6, whose
`Logger.debug`/`warn`/… methods are macros that expand to `if
(underlying.isDebugEnabled) underlying.debug(…)`. The interpolated `s"…"` (and
the `+ other` concat) is part of that guarded argument, so it isn't evaluated
when the level is disabled. Moving to SLF4J `{}` placeholders would be
non-idiomatic for this codebase and gains nothing, so I've left these as-is.
##########
amber/src/main/scala/org/apache/texera/amber/engine/common/client/ClientActor.scala:
##########
@@ -156,6 +156,6 @@ private[client] class ClientActor extends Actor with
AmberLogging {
sender() ! Ack
coordinator ! x
case other =>
- logger.warn("client actor cannot handle " + other) //skip
+ logger.debug("client actor cannot handle " + other) //skip
Review Comment:
Not needed here — these go through `com.typesafe.scala-logging` 3.9.6, whose
`Logger.debug`/`warn`/… methods are macros that expand to `if
(underlying.isDebugEnabled) underlying.debug(…)`. The interpolated `s"…"` (and
the `+ other` concat) is part of that guarded argument, so it isn't evaluated
when the level is disabled. Moving to SLF4J `{}` placeholders would be
non-idiomatic for this codebase and gains nothing, so I've left these as-is.
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/worker/DataProcessor.scala:
##########
@@ -242,14 +242,14 @@ class DataProcessor(
): Unit = {
inputManager.currentChannelId = channelId
val command = ecm.commandMapping.get(actorId.name)
- logger.info(s"receive ECM from $channelId, id = ${ecm.id}, cmd = $command")
+ logger.debug(s"receive ECM from $channelId, id = ${ecm.id}, cmd =
$command")
Review Comment:
Not needed here — these go through `com.typesafe.scala-logging` 3.9.6, whose
`Logger.debug`/`warn`/… methods are macros that expand to `if
(underlying.isDebugEnabled) underlying.debug(…)`. The interpolated `s"…"` (and
the `+ other` concat) is part of that guarded argument, so it isn't evaluated
when the level is disabled. Moving to SLF4J `{}` placeholders would be
non-idiomatic for this codebase and gains nothing, so I've left these as-is.
##########
amber/src/main/python/core/runnables/main_loop.py:
##########
@@ -453,7 +453,7 @@ def _process_ecm(self, ecm_element: ECMElement):
)
if self.context.ecm_manager.is_ecm_aligned(channel_id, ecm):
- logger.info(
+ logger.debug(
f"process channel ECM from {channel_id}, id = {ecm.id}, cmd =
{command}"
)
Review Comment:
Done in 6812804910 — switched to loguru's `{}`-placeholder form, so the
arguments are only stringified if the record is actually emitted. loguru 0.7.3
returns on `level_no < core.min_level` (`_logger.py:1900`) *before* it reaches
`message.format(*args)` (`:1991`), so with DEBUG disabled — the default, and
the CI `WARN` backstop — the `ChannelIdentity`/`command` `str()` cost is
skipped entirely.
--
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]
