You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "Roiocam (via GitHub)" <gi...@apache.org> on 2024/04/30 06:37:40 UTC
[PR] chore: avoid the double evaluation of entityId in ClusterSharding [pekko]
Roiocam opened a new pull request, #1304:
URL: https://github.com/apache/pekko/pull/1304
Found something we could optimized it.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] chore: avoid the double evaluation of entityId in ClusterSharding [pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1304:
URL: https://github.com/apache/pekko/pull/1304#discussion_r1585013146
##########
cluster-sharding/src/main/scala/org/apache/pekko/cluster/sharding/ClusterSharding.scala:
##########
@@ -425,15 +425,19 @@ class ClusterSharding(system: ExtendedActorSystem) extends Extension {
allocationStrategy: ShardAllocationStrategy,
handOffStopMessage: Any): ActorRef = {
+ val extractEntityId: ShardRegion.ExtractEntityId = { msg =>
+ messageExtractor.entityId(msg) match {
+ case eid => (eid, messageExtractor.entityMessage(msg))
Review Comment:
So does here
##########
cluster-sharding-typed/src/main/scala/org/apache/pekko/cluster/sharding/typed/internal/ClusterShardingImpl.scala:
##########
@@ -173,10 +173,10 @@ import pekko.util.JavaDurationConverters._
allocationStrategy: Option[ShardAllocationStrategy]): ActorRef[E] = {
val extractorAdapter = new ExtractorAdapter(extractor)
- val extractEntityId: ShardRegion.ExtractEntityId = {
- // TODO is it possible to avoid the double evaluation of entityId
- case message if extractorAdapter.entityId(message) != null =>
- (extractorAdapter.entityId(message), extractorAdapter.unwrapMessage(message))
+ val extractEntityId: ShardRegion.ExtractEntityId = { message =>
+ extractorAdapter.entityId(message) match {
+ case eid => (eid, extractorAdapter.unwrapMessage(message))
Review Comment:
Not sure if we still need a match for this
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] chore: avoid the double evaluation of entityId in ClusterSharding [pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #1304:
URL: https://github.com/apache/pekko/pull/1304#discussion_r1584969847
##########
cluster-sharding/src/main/scala/org/apache/pekko/cluster/sharding/ClusterSharding.scala:
##########
@@ -425,15 +425,19 @@ class ClusterSharding(system: ExtendedActorSystem) extends Extension {
allocationStrategy: ShardAllocationStrategy,
handOffStopMessage: Any): ActorRef = {
+ val extractEntityId: ShardRegion.ExtractEntityId = { msg =>
+ messageExtractor.entityId(msg) match {
+ case eid => (eid, messageExtractor.entityMessage(msg))
+ }
+ }
+ val extractShardId: ShardRegion.ExtractShardId = msg => messageExtractor.shardId(msg)
Review Comment:
Should this be privare
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] chore: avoid the double evaluation of entityId in ClusterSharding [pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #1304:
URL: https://github.com/apache/pekko/pull/1304#discussion_r1627425402
##########
cluster-sharding-typed/src/main/scala/org/apache/pekko/cluster/sharding/typed/internal/ClusterShardingImpl.scala:
##########
@@ -173,10 +174,16 @@ import pekko.util.JavaDurationConverters._
allocationStrategy: Option[ShardAllocationStrategy]): ActorRef[E] = {
val extractorAdapter = new ExtractorAdapter(extractor)
- val extractEntityId: ShardRegion.ExtractEntityId = {
- // TODO is it possible to avoid the double evaluation of entityId
- case message if extractorAdapter.entityId(message) != null =>
- (extractorAdapter.entityId(message), extractorAdapter.unwrapMessage(message))
+ // !!!important is only applicable if you know that isDefinedAt(x) is always called before apply(x) (with the same x)
+ val extractEntityId: ShardRegion.ExtractEntityId = new AbstractPartialFunction[Any, (String, Any)] {
+ var cache: String = _
+
+ override def isDefinedAt(msg: Any): Boolean = {
+ cache = extractorAdapter.entityId(msg)
+ cache != null
+ }
+
+ override def apply(x: Any): (DataCenter, Any) = (cache, extractorAdapter.unwrapMessage(x))
Review Comment:
Yep, Looks like it was a mistake.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] chore: avoid the double evaluation of entityId in ClusterSharding [pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #1304:
URL: https://github.com/apache/pekko/pull/1304#discussion_r1627484068
##########
cluster-sharding-typed/src/main/scala/org/apache/pekko/cluster/sharding/typed/internal/ClusterShardingImpl.scala:
##########
@@ -173,10 +174,16 @@ import pekko.util.JavaDurationConverters._
allocationStrategy: Option[ShardAllocationStrategy]): ActorRef[E] = {
val extractorAdapter = new ExtractorAdapter(extractor)
- val extractEntityId: ShardRegion.ExtractEntityId = {
- // TODO is it possible to avoid the double evaluation of entityId
- case message if extractorAdapter.entityId(message) != null =>
- (extractorAdapter.entityId(message), extractorAdapter.unwrapMessage(message))
+ // !!!important is only applicable if you know that isDefinedAt(x) is always called before apply(x) (with the same x)
+ val extractEntityId: ShardRegion.ExtractEntityId = new AbstractPartialFunction[Any, (String, Any)] {
+ var cache: String = _
+
+ override def isDefinedAt(msg: Any): Boolean = {
+ cache = extractorAdapter.entityId(msg)
+ cache != null
+ }
+
+ override def apply(x: Any): (DataCenter, Any) = (cache, extractorAdapter.unwrapMessage(x))
Review Comment:
Fixed, thanks!
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] chore: avoid the double evaluation of entityId in ClusterSharding [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #1304:
URL: https://github.com/apache/pekko/pull/1304#issuecomment-2147218593
@Roiocam is it ok to get this merged now?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] chore: avoid the double evaluation of entityId in ClusterSharding [pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on PR #1304:
URL: https://github.com/apache/pekko/pull/1304#issuecomment-2106689353
Use the cacheable partial function, and then verify it works.
```scala
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")
```
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] chore: avoid the double evaluation of entityId in ClusterSharding [pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam merged PR #1304:
URL: https://github.com/apache/pekko/pull/1304
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] chore: avoid the double evaluation of entityId in ClusterSharding [pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #1304:
URL: https://github.com/apache/pekko/pull/1304#discussion_r1585222400
##########
cluster-sharding/src/main/scala/org/apache/pekko/cluster/sharding/ClusterSharding.scala:
##########
@@ -425,15 +425,19 @@ class ClusterSharding(system: ExtendedActorSystem) extends Extension {
allocationStrategy: ShardAllocationStrategy,
handOffStopMessage: Any): ActorRef = {
+ val extractEntityId: ShardRegion.ExtractEntityId = { msg =>
+ messageExtractor.entityId(msg) match {
+ case eid => (eid, messageExtractor.entityMessage(msg))
+ }
+ }
+ val extractShardId: ShardRegion.ExtractShardId = msg => messageExtractor.shardId(msg)
Review Comment:
Thanks, i think this PR wasn't correct. I will find another solution fix this.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] chore: avoid the double evaluation of entityId in ClusterSharding [pekko]
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on code in PR #1304:
URL: https://github.com/apache/pekko/pull/1304#discussion_r1626950311
##########
cluster-sharding-typed/src/main/scala/org/apache/pekko/cluster/sharding/typed/internal/ClusterShardingImpl.scala:
##########
@@ -173,10 +174,16 @@ import pekko.util.JavaDurationConverters._
allocationStrategy: Option[ShardAllocationStrategy]): ActorRef[E] = {
val extractorAdapter = new ExtractorAdapter(extractor)
- val extractEntityId: ShardRegion.ExtractEntityId = {
- // TODO is it possible to avoid the double evaluation of entityId
- case message if extractorAdapter.entityId(message) != null =>
- (extractorAdapter.entityId(message), extractorAdapter.unwrapMessage(message))
+ // !!!important is only applicable if you know that isDefinedAt(x) is always called before apply(x) (with the same x)
+ val extractEntityId: ShardRegion.ExtractEntityId = new AbstractPartialFunction[Any, (String, Any)] {
+ var cache: String = _
+
+ override def isDefinedAt(msg: Any): Boolean = {
+ cache = extractorAdapter.entityId(msg)
+ cache != null
+ }
+
+ override def apply(x: Any): (DataCenter, Any) = (cache, extractorAdapter.unwrapMessage(x))
Review Comment:
Shouldn't that be `(String, Any)`?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] chore: avoid the double evaluation of entityId in ClusterSharding [pekko]
Posted by "laglangyue (via GitHub)" <gi...@apache.org>.
laglangyue commented on PR #1304:
URL: https://github.com/apache/pekko/pull/1304#issuecomment-2087987109
Seems like duplicate code, can it be abstracted
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] chore: avoid the double evaluation of entityId in ClusterSharding [pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on PR #1304:
URL: https://github.com/apache/pekko/pull/1304#issuecomment-2147689418
> @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.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org