-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: avoid the double evaluation of entityId in ClusterSharding #1304
Conversation
cluster-sharding/src/main/scala/org/apache/pekko/cluster/sharding/ClusterSharding.scala
Outdated
Show resolved
Hide resolved
...ed/src/main/scala/org/apache/pekko/cluster/sharding/typed/internal/ClusterShardingImpl.scala
Outdated
Show resolved
Hide resolved
cluster-sharding/src/main/scala/org/apache/pekko/cluster/sharding/ClusterSharding.scala
Outdated
Show resolved
Hide resolved
Seems like duplicate code, can it be abstracted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blocking - needs discussion on the dev list because of the ongoing release discussion
Use the cacheable partial function, and then verify it works. var calTime: Int = 0
def entityId(value: Any): String = {
calTime = calTime + 1
value match {
case "wrong" => null
case _ => String.valueOf(value)
}
}
def extractEntityIdFromExtractor: PartialFunction[Any, Any] =
new scala.runtime.AbstractPartialFunction[Any, (String, Any)] {
var cache: String = _
override def isDefinedAt(msg: Any): Boolean = {
cache = entityId(msg)
cache != null
}
override def apply(x: Any): (String, Any) = (cache, x)
}
lazy val xx = extractEntityIdFromExtractor
lazy val yy: PartialFunction[Any, Any] = {
case message if entityId(message) != null => (entityId(message), message)
}
assert(!xx.isDefinedAt("wrong"))
assert(!yy.isDefinedAt("wrong"))
calTime = 0
val health = "health"
println(xx.isDefinedAt(health))
println(xx(health))
println(s"caltime = $calTime")
println("=====")
calTime = 0
println(yy.isDefinedAt(health))
println(yy(health))
println(s"caltime = $calTime")
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@Roiocam is it ok to get this merged now? |
I have no objections to this, but it would be better if there were reviews from others. |
...ed/src/main/scala/org/apache/pekko/cluster/sharding/typed/internal/ClusterShardingImpl.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
…ache#1304) * chore: avoid the double evaluation of entityId in ClusterSharding * new cacheable partial function * optimized for review * fix the right type
…ache#1304) * chore: avoid the double evaluation of entityId in ClusterSharding * new cacheable partial function * optimized for review * fix the right type
* add unit test protect ExtractEntityId can be shared safely Related with #1463 * chore: avoid the double evaluation of entityId in ClusterSharding (#1304) * chore: avoid the double evaluation of entityId in ClusterSharding * new cacheable partial function * optimized for review * fix the right type * Revert "chore: avoid the double evaluation of entityId in ClusterSharding (#1…" (#1464) This reverts commit b0e9886. * grammar fix * sort imports --------- Co-authored-by: PJ Fanning <[email protected]>
Found something we could optimized it.