He-Pin commented on code in PR #3058:
URL: https://github.com/apache/pekko/pull/3058#discussion_r3407967851
##########
persistence/src/main/scala/org/apache/pekko/persistence/state/DurableStateStoreRegistry.scala:
##########
@@ -91,6 +95,17 @@ class DurableStateStoreRegistry(system: ExtendedActorSystem)
pluginFor(pluginIdOrDefault(pluginId),
pluginConfig(pluginId)).scaladslPlugin.asInstanceOf[T]
}
+ /**
+ * Scala API: Returns the
[[pekko.persistence.state.scaladsl.DurableStateStore]] specified by the given
+ * configuration entry. The provided `pluginConfig` is used to configure the
plugin at runtime, taking
+ * precedence over the plugin configuration defined in the actor system
configuration.
+ *
+ * @since 2.0.0
Review Comment:
🟡 **Parameter name shadows private method**
The parameter `pluginConfig: Config` shadows the private method
`pluginConfig(pluginId: String)` defined at line 76. While the current method
body doesn't call the private method (so no compile error), this creates a
maintenance trap: a future edit adding `pluginConfig(someId)` here would get a
confusing *"Config is not callable"* error instead of invoking the private
method.
Same concern applies to `getDurableStateStoreFor` at line 130.
**Suggested fix** — rename the parameter to avoid the collision:
```scala
final def durableStateStoreFor[T <: scaladsl.DurableStateStore[?]](pluginId:
String, runtimeConfig: Config): T = {
pluginFor(pluginIdOrDefault(pluginId, runtimeConfig),
runtimeConfig).scaladslPlugin.asInstanceOf[T]
}
```
##########
persistence-typed-tests/src/test/scala/org/apache/pekko/persistence/typed/state/scaladsl/RuntimeDurableStateStoreSpec.scala:
##########
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.pekko.persistence.typed.state.scaladsl
+
+import org.apache.pekko
+import pekko.Done
+import pekko.actor.testkit.typed.scaladsl.LogCapturing
+import pekko.actor.testkit.typed.scaladsl.ScalaTestWithActorTestKit
+import pekko.actor.typed.ActorRef
+import pekko.actor.typed.Behavior
+import pekko.persistence.state.DurableStateStoreRegistry
+import pekko.persistence.state.scaladsl.DurableStateUpdateStore
+import
pekko.persistence.testkit.state.PersistenceTestKitDurableStateStoreProvider
+import pekko.persistence.typed.PersistenceId
+
+import com.typesafe.config.Config
+import com.typesafe.config.ConfigFactory
+import org.scalatest.wordspec.AnyWordSpecLike
+
+object RuntimeDurableStateStoreSpec {
+
+ private object Actor {
+ sealed trait Command
+ final case class Save(text: String, replyTo: ActorRef[Done]) extends
Command
+ final case class ShowMeWhatYouGot(replyTo: ActorRef[String]) extends
Command
+ case object Stop extends Command
+
+ def apply(persistenceId: String, store: String): Behavior[Command] =
+ DurableStateBehavior[Command, String](
+ PersistenceId.ofUniqueId(persistenceId),
+ "",
+ (state, cmd) =>
+ cmd match {
+ case Save(text, replyTo) =>
+ Effect.persist(Seq(state,
text).filter(_.nonEmpty).mkString("|")).thenRun(_ => replyTo ! Done)
+ case ShowMeWhatYouGot(replyTo) =>
+ replyTo ! state
+ Effect.none
+ case Stop =>
+ Effect.stop()
+ })
+ .withDurableStateStorePluginId(s"$store.state")
+ .withDurableStateStorePluginConfig(Some(config(store)))
+ }
+
+ private def config(store: String): Config =
+ ConfigFactory.parseString(s"""
+ $store {
+ state.class =
"${classOf[PersistenceTestKitDurableStateStoreProvider].getName}"
+ }
Review Comment:
🟡 **Test only covers Scala API**
This spec exercises `DurableStateBehavior.withDurableStateStorePluginConfig`
(Scala DSL) and `DurableStateStoreRegistry.durableStateStoreFor(pluginId,
config)` (Scala registry API).
The Java DSL counterpart
(`DurableStateBehavior.durableStateStorePluginConfig()` returning
`Optional[Config]`) and the Java registry API (`getDurableStateStoreFor(clazz,
pluginId, config)`) are untested.
Per `CONTRIBUTING.md`: *"Scala and Java DSL changes must keep API, docs, and
tests in parity."*
**Suggested fix** — add a Java DSL spec (e.g.,
`RuntimeDurableStateStoreJavaSpec`) that defines a `DurableStateBehavior`
subclass overriding `durableStateStorePluginConfig()`, and verifies store
isolation via `getDurableStateStoreFor`.
--
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]
