You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "nvollmar (via GitHub)" <gi...@apache.org> on 2023/03/10 12:17:37 UTC
[GitHub] [incubator-pekko-persistence-cassandra] nvollmar opened a new pull request, #26: Fixes compile warnings
nvollmar opened a new pull request, #26:
URL: https://github.com/apache/incubator-pekko-persistence-cassandra/pull/26
fixes some warnings
adds missing headers
--
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-persistence-cassandra] mdedetrich commented on a diff in pull request #26: Fixes compile warnings
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #26:
URL: https://github.com/apache/incubator-pekko-persistence-cassandra/pull/26#discussion_r1132489302
##########
core/src/main/scala/akka/persistence/cassandra/snapshot/CassandraSnapshotStore.scala:
##########
@@ -133,10 +133,10 @@ import akka.stream.alpakka.cassandra.scaladsl.{ CassandraSession, CassandraSessi
case md +: mds =>
load1Async(md)
.map {
- case DeserializedSnapshot(payload, OptionVal.Some(snapshotMeta)) =>
- Some(SelectedSnapshot(md.withMetadata(snapshotMeta), payload))
case DeserializedSnapshot(payload, OptionVal.None) =>
Some(SelectedSnapshot(md, payload))
+ case DeserializedSnapshot(payload, some) =>
Review Comment:
Same comment as https://github.com/apache/incubator-pekko-persistence-cassandra/pull/26#discussion_r1132486518
--
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-persistence-cassandra] pjfanning commented on a diff in pull request #26: Fixes compile warnings
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #26:
URL: https://github.com/apache/incubator-pekko-persistence-cassandra/pull/26#discussion_r1132532762
##########
core/src/main/scala/akka/persistence/cassandra/snapshot/CassandraSnapshotStore.scala:
##########
@@ -133,10 +133,10 @@ import akka.stream.alpakka.cassandra.scaladsl.{ CassandraSession, CassandraSessi
case md +: mds =>
load1Async(md)
.map {
- case DeserializedSnapshot(payload, OptionVal.Some(snapshotMeta)) =>
- Some(SelectedSnapshot(md.withMetadata(snapshotMeta), payload))
case DeserializedSnapshot(payload, OptionVal.None) =>
Some(SelectedSnapshot(md, payload))
+ case DeserializedSnapshot(payload, some) =>
Review Comment:
If the issue is very unlikely to occur, maybe best to wait to change. Another option is to have a config that controls whether you get old behaviour or new behaviour.
--
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-persistence-cassandra] nvollmar commented on pull request #26: Fixes compile warnings
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on PR #26:
URL: https://github.com/apache/incubator-pekko-persistence-cassandra/pull/26#issuecomment-1477301036
@mdedetrich this would be ready for review
--
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-persistence-cassandra] mdedetrich commented on a diff in pull request #26: Fixes compile warnings
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #26:
URL: https://github.com/apache/incubator-pekko-persistence-cassandra/pull/26#discussion_r1132486518
##########
core/src/main/scala/akka/persistence/cassandra/journal/CassandraJournal.scala:
##########
@@ -800,9 +794,8 @@ import akka.stream.scaladsl.Source
Extractors.optionalTaggedPersistentRepr(eventDeserializer, serialization))
.mapAsync(1) { t =>
t.tagged match {
- case OptionVal.Some(tpr) =>
- tr.sendMissingTagWrite(tp)(tpr)
case OptionVal.None => FutureDone // no tags, skip
Review Comment:
If it is a bug I would be a bit more comfortable changing this for 1.1.x rather than 1.1.0 (the premise being if it was critical bug it likely would have been solved by now).
@pjfanning @jrudolph @raboof wdyt?
--
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-persistence-cassandra] mdedetrich commented on a diff in pull request #26: Fixes compile warnings
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #26:
URL: https://github.com/apache/incubator-pekko-persistence-cassandra/pull/26#discussion_r1132486518
##########
core/src/main/scala/akka/persistence/cassandra/journal/CassandraJournal.scala:
##########
@@ -800,9 +794,8 @@ import akka.stream.scaladsl.Source
Extractors.optionalTaggedPersistentRepr(eventDeserializer, serialization))
.mapAsync(1) { t =>
t.tagged match {
- case OptionVal.Some(tpr) =>
- tr.sendMissingTagWrite(tp)(tpr)
case OptionVal.None => FutureDone // no tags, skip
Review Comment:
If it is a bug I would be a bit more comfortable changing this for 1.1.x rather than 1.1.0 (the premise being if it was critical bug it likely would have been solved by now). It might also be possible that someone is inadvertently relying on this behaviour and we don't want more surprises then necessary in a jump from Akka to Pekko.
@pjfanning @jrudolph @raboof wdyt?
--
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-persistence-cassandra] nvollmar commented on pull request #26: Fixes compile warnings
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on PR #26:
URL: https://github.com/apache/incubator-pekko-persistence-cassandra/pull/26#issuecomment-1465235505
Makes sense, I'll split that in two 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-persistence-cassandra] nvollmar commented on a diff in pull request #26: Fixes compile warnings
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on code in PR #26:
URL: https://github.com/apache/incubator-pekko-persistence-cassandra/pull/26#discussion_r1132302451
##########
core/src/main/scala/akka/persistence/cassandra/snapshot/CassandraSnapshotStore.scala:
##########
@@ -133,10 +133,10 @@ import akka.stream.alpakka.cassandra.scaladsl.{ CassandraSession, CassandraSessi
case md +: mds =>
load1Async(md)
.map {
- case DeserializedSnapshot(payload, OptionVal.Some(snapshotMeta)) =>
- Some(SelectedSnapshot(md.withMetadata(snapshotMeta), payload))
case DeserializedSnapshot(payload, OptionVal.None) =>
Some(SelectedSnapshot(md, payload))
+ case DeserializedSnapshot(payload, some) =>
Review Comment:
Same 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-persistence-cassandra] nvollmar commented on a diff in pull request #26: Fixes compile warnings
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on code in PR #26:
URL: https://github.com/apache/incubator-pekko-persistence-cassandra/pull/26#discussion_r1132300968
##########
core/src/main/scala/akka/persistence/cassandra/journal/CassandraJournal.scala:
##########
@@ -800,9 +794,8 @@ import akka.stream.scaladsl.Source
Extractors.optionalTaggedPersistentRepr(eventDeserializer, serialization))
.mapAsync(1) { t =>
t.tagged match {
- case OptionVal.Some(tpr) =>
- tr.sendMissingTagWrite(tp)(tpr)
case OptionVal.None => FutureDone // no tags, skip
Review Comment:
IMO this was a bug (None is defined as `OptionalVal(null)`) and matching first on `Some(x)` would match `OptionalVal(null)` too.
--
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-persistence-cassandra] nvollmar merged pull request #26: Fixes compile warnings
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar merged PR #26:
URL: https://github.com/apache/incubator-pekko-persistence-cassandra/pull/26
--
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-persistence-cassandra] nvollmar commented on a diff in pull request #26: Fixes compile warnings
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on code in PR #26:
URL: https://github.com/apache/incubator-pekko-persistence-cassandra/pull/26#discussion_r1132300968
##########
core/src/main/scala/akka/persistence/cassandra/journal/CassandraJournal.scala:
##########
@@ -800,9 +794,8 @@ import akka.stream.scaladsl.Source
Extractors.optionalTaggedPersistentRepr(eventDeserializer, serialization))
.mapAsync(1) { t =>
t.tagged match {
- case OptionVal.Some(tpr) =>
- tr.sendMissingTagWrite(tp)(tpr)
case OptionVal.None => FutureDone // no tags, skip
Review Comment:
IMO this was a bug (None is defined as `OptionalVal(null)`) and matching first on `Some(x)` would match `Some(null)` too.
--
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-persistence-cassandra] mdedetrich commented on a diff in pull request #26: Fixes compile warnings
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #26:
URL: https://github.com/apache/incubator-pekko-persistence-cassandra/pull/26#discussion_r1132534367
##########
core/src/main/scala/akka/persistence/cassandra/snapshot/CassandraSnapshotStore.scala:
##########
@@ -133,10 +133,10 @@ import akka.stream.alpakka.cassandra.scaladsl.{ CassandraSession, CassandraSessi
case md +: mds =>
load1Async(md)
.map {
- case DeserializedSnapshot(payload, OptionVal.Some(snapshotMeta)) =>
- Some(SelectedSnapshot(md.withMetadata(snapshotMeta), payload))
case DeserializedSnapshot(payload, OptionVal.None) =>
Some(SelectedSnapshot(md, payload))
+ case DeserializedSnapshot(payload, some) =>
Review Comment:
I think a config is overkill. We should verify what the behaviour should be and if its different in my view it should go into 1.1.x
--
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-persistence-cassandra] nvollmar commented on pull request #26: Fixes compile warnings
Posted by "nvollmar (via GitHub)" <gi...@apache.org>.
nvollmar commented on PR #26:
URL: https://github.com/apache/incubator-pekko-persistence-cassandra/pull/26#issuecomment-1467375949
@mdedetrich I split 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