You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by GitBox <gi...@apache.org> on 2023/01/09 06:48:40 UTC
[GitHub] [incubator-pekko] nvollmar opened a new pull request, #94: Adds whitelist for custom serializer of classes in pekko package
nvollmar opened a new pull request, #94:
URL: https://github.com/apache/incubator-pekko/pull/94
We implemented this in our internal Akka fork until we can migrate to Pekko.
The query view implementation need access to certain Actor internals and therefore has to reside within the akka/pekko package. As we want to use our persistence serializer for snapshots, this normally generates a warning which we want to ignore.
--
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
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #94: Adds whitelist for custom serializer of classes in pekko package
Posted by GitBox <gi...@apache.org>.
pjfanning commented on code in PR #94:
URL: https://github.com/apache/incubator-pekko/pull/94#discussion_r1064481159
##########
actor/src/main/scala/org/apache/pekko/serialization/Serialization.scala:
##########
@@ -457,8 +454,12 @@ class Serialization(val system: ExtendedActorSystem) extends Extension {
}
}
+ @nowarn("msg=deprecated")
private def warnUnexpectedNonAkkaSerializer(clazz: Class[_], ser: Serializer): Boolean = {
Review Comment:
Could we change the function name to use 'NonPekko'?
##########
actor/src/main/resources/reference.conf:
##########
@@ -739,6 +739,9 @@ pekko {
# to reduce noise.
warn-on-no-serialization-verification = on
+ # list of fqcn of classes that may use non-pekko serializer within pekko package without warn log
+ warn-non-pekko-serializer-whitelist = ${?akka.actor.warn-non-pekko-serializer-whitelist} []
Review Comment:
Could we call this an `allow-list` instead of `whitelist` - to avoid the connotations about colors?
--
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
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #94: Adds whitelist for custom serializer of classes in pekko package
Posted by GitBox <gi...@apache.org>.
He-Pin commented on code in PR #94:
URL: https://github.com/apache/incubator-pekko/pull/94#discussion_r1067156388
##########
actor/src/main/scala/org/apache/pekko/serialization/Serialization.scala:
##########
@@ -457,8 +453,12 @@ class Serialization(val system: ExtendedActorSystem) extends Extension {
}
}
- private def warnUnexpectedNonAkkaSerializer(clazz: Class[_], ser: Serializer): Boolean = {
- if (clazz.getName.startsWith("org.apache.pekko.") && !ser.getClass.getName.startsWith("org.apache.pekko.")) {
+ @nowarn("msg=deprecated")
+ private def warnUnexpectedNonPekkoSerializer(clazz: Class[_], ser: Serializer): Boolean = {
+ import scala.collection.JavaConverters._ // switch to scala.jdk.CollectionConverters once Scala 2.12 support is dropped
+ if (clazz.getName.startsWith("org.apache.pekko.") && !ser.getClass.getName.startsWith("org.apache.pekko.") &&
Review Comment:
extract `"org.apache.pekko.` to a constant field?
--
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
[GitHub] [incubator-pekko] nvollmar commented on pull request #94: Adds whitelist for custom serializer of classes in pekko package
Posted by GitBox <gi...@apache.org>.
nvollmar commented on PR #94:
URL: https://github.com/apache/incubator-pekko/pull/94#issuecomment-1375249555
refs #95
--
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] Adds whitelist for custom serializer of classes in pekko package [incubator-pekko]
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar closed pull request #94: Adds whitelist for custom serializer of classes in pekko package
URL: https://github.com/apache/incubator-pekko/pull/94
--
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
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #94: Adds whitelist for custom serializer of classes in pekko package
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #94:
URL: https://github.com/apache/incubator-pekko/pull/94#discussion_r1087816708
##########
actor/src/main/scala/org/apache/pekko/serialization/Serialization.scala:
##########
@@ -457,8 +453,12 @@ class Serialization(val system: ExtendedActorSystem) extends Extension {
}
}
- private def warnUnexpectedNonAkkaSerializer(clazz: Class[_], ser: Serializer): Boolean = {
- if (clazz.getName.startsWith("org.apache.pekko.") && !ser.getClass.getName.startsWith("org.apache.pekko.")) {
+ @nowarn("msg=deprecated")
+ private def warnUnexpectedNonPekkoSerializer(clazz: Class[_], ser: Serializer): Boolean = {
+ import scala.collection.JavaConverters._ // switch to scala.jdk.CollectionConverters once Scala 2.12 support is dropped
+ if (clazz.getName.startsWith("org.apache.pekko.") && !ser.getClass.getName.startsWith("org.apache.pekko.") &&
Review Comment:
This might make sense in a separate PR, at least when I was renaming the package to `org.apache.pekko` there were a few cases where package names was being checked `clazz.getName`?
--
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
[GitHub] [incubator-pekko] nvollmar commented on pull request #94: Adds whitelist for custom serializer of classes in pekko package
Posted by GitBox <gi...@apache.org>.
nvollmar commented on PR #94:
URL: https://github.com/apache/incubator-pekko/pull/94#issuecomment-1375551921
> would it be possible to add a unit test?
I can look into that, just the feature to write that log doesn't have a test in the first place. Do we expect tests for logs?
--
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
[GitHub] [incubator-pekko] nvollmar commented on pull request #94: Adds whitelist for custom serializer of classes in pekko package
Posted by GitBox <gi...@apache.org>.
nvollmar commented on PR #94:
URL: https://github.com/apache/incubator-pekko/pull/94#issuecomment-1375225980
I can open an issue matching the PR
--
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
[GitHub] [incubator-pekko] nvollmar commented on a diff in pull request #94: Adds whitelist for custom serializer of classes in pekko package
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on code in PR #94:
URL: https://github.com/apache/incubator-pekko/pull/94#discussion_r1291099860
##########
actor/src/main/resources/reference.conf:
##########
@@ -741,6 +741,9 @@ pekko {
# to reduce noise.
warn-on-no-serialization-verification = on
+ # list of fqcn of classes that may use non-pekko serializer within pekko package without warn log
+ warn-non-pekko-serializer-allow-list = ${?pekko.actor.warn-non-pekko-serializer-allow-list} []
Review Comment:
added
--
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
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #94: Adds whitelist for custom serializer of classes in pekko package
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #94:
URL: https://github.com/apache/incubator-pekko/pull/94#discussion_r1290424953
##########
actor/src/main/resources/reference.conf:
##########
@@ -741,6 +741,9 @@ pekko {
# to reduce noise.
warn-on-no-serialization-verification = on
+ # list of fqcn of classes that may use non-pekko serializer within pekko package without warn log
+ warn-non-pekko-serializer-allow-list = ${?pekko.actor.warn-non-pekko-serializer-allow-list} []
Review Comment:
might be worth updating the comment to mention that this is new to Pekko v1.1.0
--
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] Adds whitelist for custom serializer of classes in pekko package [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #94:
URL: https://github.com/apache/incubator-pekko/pull/94#discussion_r1365782961
##########
actor/src/main/scala/org/apache/pekko/serialization/Serialization.scala:
##########
@@ -466,8 +462,12 @@ class Serialization(val system: ExtendedActorSystem) extends Extension {
}
}
+ @nowarn("msg=deprecated")
private def warnUnexpectedNonPekkoSerializer(clazz: Class[_], ser: Serializer): Boolean = {
- if (clazz.getName.startsWith("org.apache.pekko.") && !ser.getClass.getName.startsWith("org.apache.pekko.")) {
+ import pekko.util.ccompat.JavaConverters._
+ if (clazz.getName.startsWith("org.apache.pekko.") && !ser.getClass.getName.startsWith("org.apache.pekko.") &&
+ !system.settings.config.getStringList("pekko.actor.warn-non-pekko-serializer-allow-list").asScala.toSet.contains(
Review Comment:
this line did't use too many feature of scala, maybe it doesn't need to convert to scala and Set
--
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
[GitHub] [incubator-pekko] He-Pin commented on pull request #94: Adds whitelist for custom serializer of classes in pekko package
Posted by GitBox <gi...@apache.org>.
He-Pin commented on PR #94:
URL: https://github.com/apache/incubator-pekko/pull/94#issuecomment-1375217594
refs to an issue for this PR?
--
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
[GitHub] [incubator-pekko] nvollmar commented on a diff in pull request #94: Adds whitelist for custom serializer of classes in pekko package
Posted by GitBox <gi...@apache.org>.
nvollmar commented on code in PR #94:
URL: https://github.com/apache/incubator-pekko/pull/94#discussion_r1064525755
##########
actor/src/main/resources/reference.conf:
##########
@@ -739,6 +739,9 @@ pekko {
# to reduce noise.
warn-on-no-serialization-verification = on
+ # list of fqcn of classes that may use non-pekko serializer within pekko package without warn log
+ warn-non-pekko-serializer-whitelist = ${?akka.actor.warn-non-pekko-serializer-whitelist} []
Review Comment:
sure... tried to follow the pattern of other config entries which use 'whitelist'
--
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
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #94: Adds whitelist for custom serializer of classes in pekko package
Posted by GitBox <gi...@apache.org>.
He-Pin commented on code in PR #94:
URL: https://github.com/apache/incubator-pekko/pull/94#discussion_r1067155166
##########
actor/src/main/scala/org/apache/pekko/serialization/Serialization.scala:
##########
@@ -457,8 +453,12 @@ class Serialization(val system: ExtendedActorSystem) extends Extension {
}
}
- private def warnUnexpectedNonAkkaSerializer(clazz: Class[_], ser: Serializer): Boolean = {
- if (clazz.getName.startsWith("org.apache.pekko.") && !ser.getClass.getName.startsWith("org.apache.pekko.")) {
+ @nowarn("msg=deprecated")
+ private def warnUnexpectedNonPekkoSerializer(clazz: Class[_], ser: Serializer): Boolean = {
+ import scala.collection.JavaConverters._ // switch to scala.jdk.CollectionConverters once Scala 2.12 support is dropped
Review Comment:
add a TODO here?
--
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
[GitHub] [incubator-pekko] nvollmar commented on a diff in pull request #94: Adds whitelist for custom serializer of classes in pekko package
Posted by GitBox <gi...@apache.org>.
nvollmar commented on code in PR #94:
URL: https://github.com/apache/incubator-pekko/pull/94#discussion_r1064523745
##########
actor/src/main/scala/org/apache/pekko/serialization/Serialization.scala:
##########
@@ -457,8 +454,12 @@ class Serialization(val system: ExtendedActorSystem) extends Extension {
}
}
+ @nowarn("msg=deprecated")
private def warnUnexpectedNonAkkaSerializer(clazz: Class[_], ser: Serializer): Boolean = {
Review Comment:
Makes sense
--
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
[GitHub] [incubator-pekko] nvollmar commented on a diff in pull request #94: Adds whitelist for custom serializer of classes in pekko package
Posted by GitBox <gi...@apache.org>.
nvollmar commented on code in PR #94:
URL: https://github.com/apache/incubator-pekko/pull/94#discussion_r1064525755
##########
actor/src/main/resources/reference.conf:
##########
@@ -739,6 +739,9 @@ pekko {
# to reduce noise.
warn-on-no-serialization-verification = on
+ # list of fqcn of classes that may use non-pekko serializer within pekko package without warn log
+ warn-non-pekko-serializer-whitelist = ${?akka.actor.warn-non-pekko-serializer-whitelist} []
Review Comment:
sure...
--
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
[GitHub] [incubator-pekko] nvollmar commented on pull request #94: Adds whitelist for custom serializer of classes in pekko package
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on PR #94:
URL: https://github.com/apache/incubator-pekko/pull/94#issuecomment-1672869161
Just updated to latest main.
@jrudolph Didn't see your comment back then. The QueryView requires access to certain pekko private methods to implement snapshot/event recovery and so on.
--
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] Adds whitelist for custom serializer of classes in pekko package [incubator-pekko]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #94:
URL: https://github.com/apache/incubator-pekko/pull/94#discussion_r1365752172
##########
actor/src/main/scala/org/apache/pekko/serialization/Serialization.scala:
##########
@@ -466,8 +462,12 @@ class Serialization(val system: ExtendedActorSystem) extends Extension {
}
}
+ @nowarn("msg=deprecated")
private def warnUnexpectedNonPekkoSerializer(clazz: Class[_], ser: Serializer): Boolean = {
- if (clazz.getName.startsWith("org.apache.pekko.") && !ser.getClass.getName.startsWith("org.apache.pekko.")) {
Review Comment:
I don't think it is good idea to turn off these warnning, if have to, better using same way like `shouldWarnAboutJavaSerializer` rather than using a list struct.
--
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] Adds whitelist for custom serializer of classes in pekko package [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #94:
URL: https://github.com/apache/incubator-pekko/pull/94#discussion_r1456955221
##########
actor/src/main/resources/reference.conf:
##########
@@ -741,6 +741,9 @@ pekko {
# to reduce noise.
warn-on-no-serialization-verification = on
+ # list of fqcn of classes that may use non-pekko serializer within pekko package without warn log
+ warn-non-pekko-serializer-allow-list = ${?pekko.actor.warn-non-pekko-serializer-allow-list} []
Review Comment:
What's the usercase for this :) I'm asking because I'm not a persistence user:)
##########
actor/src/main/resources/reference.conf:
##########
@@ -741,6 +741,9 @@ pekko {
# to reduce noise.
warn-on-no-serialization-verification = on
+ # list of fqcn of classes that may use non-pekko serializer within pekko package without warn log
+ warn-non-pekko-serializer-allow-list = ${?pekko.actor.warn-non-pekko-serializer-allow-list} []
Review Comment:
What's the use case for this :) I'm asking because I'm not a persistence user:)
--
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