You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "spangaer (via GitHub)" <gi...@apache.org> on 2023/02/10 14:14:28 UTC
[GitHub] [incubator-pekko-persistence-dynamodb] spangaer opened a new pull request, #13: Protect against doing requests for snapshots that are over 400KB.
spangaer opened a new pull request, #13:
URL: https://github.com/apache/incubator-pekko-persistence-dynamodb/pull/13
Sources:
https://zaccharles.medium.com/calculating-a-dynamodb-items-size-and-consumed-capacity-d1728942eb7c https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/CapacityUnitCalculations.html
**Draft note**
I'm well aware that the rename refactor and deployment processes are still ongoing. We'll adapt these commits to the new reality as it materializes but wanted to stage the contributions already.
Also FYI @coreyoconnor
--
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-dynamodb] spangaer commented on a diff in pull request #13: Protect against doing requests for snapshots that are over 400KB.
Posted by "spangaer (via GitHub)" <gi...@apache.org>.
spangaer commented on code in PR #13:
URL: https://github.com/apache/incubator-pekko-persistence-dynamodb/pull/13#discussion_r1170442486
##########
src/main/scala/akka/persistence/dynamodb/snapshot/DynamoDBSnapshotStore.scala:
##########
@@ -12,6 +12,8 @@ import com.typesafe.config.Config
import akka.persistence.dynamodb._
import scala.concurrent.Future
+class DynamoDBSnapshotRejection(message: String, cause: Throwable = null) extends RuntimeException(message, cause)
Review Comment:
Seems like that then breaks consistency with e.g.
https://github.com/esko-oss/pekko-persistence-dynamodb/blob/4d172134208279a00b90ef597dd0970c31b56fde/src/main/scala/akka/persistence/dynamodb/journal/DynamoDBJournal.scala#L30
If this exception ripples back to the persistent actor, the persistence ID would be known there 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-dynamodb] mdedetrich commented on a diff in pull request #13: Protect against doing requests for snapshots that are over 400KB.
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #13:
URL: https://github.com/apache/incubator-pekko-persistence-dynamodb/pull/13#discussion_r1117455690
##########
src/main/scala/akka/persistence/dynamodb/snapshot/DynamoDBSnapshotStore.scala:
##########
@@ -12,6 +12,8 @@ import com.typesafe.config.Config
import akka.persistence.dynamodb._
import scala.concurrent.Future
+class DynamoDBSnapshotRejection(message: String, cause: Throwable = null) extends RuntimeException(message, cause)
Review Comment:
Can we add a `Key` field as a `val` to this exception so we know which key caused the exception incase people want to catch it.
Also mention the key in the exception message.
--
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-dynamodb] mdedetrich commented on a diff in pull request #13: Protect against doing requests for snapshots that are over 400KB.
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #13:
URL: https://github.com/apache/incubator-pekko-persistence-dynamodb/pull/13#discussion_r1117458402
##########
src/main/scala/akka/persistence/dynamodb/snapshot/package.scala:
##########
@@ -21,4 +21,21 @@ package object snapshot {
val Payload = "pay"
val SequenceNr = "seq"
+ /**
+ * Returns (a slightly overestimated) size of the fixed fields in a snapshot DynamoDB record.
+ *
+ * Assumes the maximum of 21 bytes for a number.
+ *
+ * Sources
+ * https://zaccharles.medium.com/calculating-a-dynamodb-items-size-and-consumed-capacity-d1728942eb7c
+ * https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/CapacityUnitCalculations.html
+ */
+ val dynamoFixedByteSize =
Review Comment:
Should be `DynamoFixedByteSize` since its a constant (Scala convention).
##########
src/main/scala/akka/persistence/dynamodb/snapshot/DynamoDBSnapshotStore.scala:
##########
@@ -12,6 +12,8 @@ import com.typesafe.config.Config
import akka.persistence.dynamodb._
import scala.concurrent.Future
+class DynamoDBSnapshotRejection(message: String, cause: Throwable = null) extends RuntimeException(message, cause)
Review Comment:
Can we add a `Key` field as a `val` to this exception so we know which key caused the exception incase people want to catch it.
Also mention the key in the exception message.
--
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] Protect against doing requests for snapshots that are over 400KB. [incubator-pekko-persistence-dynamodb]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #13:
URL: https://github.com/apache/incubator-pekko-persistence-dynamodb/pull/13#discussion_r1430574857
##########
src/test/scala/org/apache/pekko/persistence/dynamodb/snapshot/SnapshotTooBigSpec.scala:
##########
@@ -0,0 +1,62 @@
+/*
Review Comment:
new files should use the standard ASF header like the one in https://github.com/apache/incubator-pekko/blob/main/project/AddMetaInfLicenseFiles.scala
only if the code is largely copied from Akka should it have the special 'derived from Akka' header
--
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] Protect against doing requests for snapshots that are over 400KB. [incubator-pekko-persistence-dynamodb]
Posted by "spangaer (via GitHub)" <gi...@apache.org>.
spangaer commented on PR #13:
URL: https://github.com/apache/incubator-pekko-persistence-dynamodb/pull/13#issuecomment-1862335166
Should be fixed now.
Failure to use `scalafmtAll` instead of `scalafmt`.
--
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-dynamodb] spangaer commented on a diff in pull request #13: Protect against doing requests for snapshots that are over 400KB.
Posted by "spangaer (via GitHub)" <gi...@apache.org>.
spangaer commented on code in PR #13:
URL: https://github.com/apache/incubator-pekko-persistence-dynamodb/pull/13#discussion_r1170441514
##########
src/main/scala/akka/persistence/dynamodb/snapshot/package.scala:
##########
@@ -21,4 +21,21 @@ package object snapshot {
val Payload = "pay"
val SequenceNr = "seq"
+ /**
+ * Returns (a slightly overestimated) size of the fixed fields in a snapshot DynamoDB record.
+ *
+ * Assumes the maximum of 21 bytes for a number.
+ *
+ * Sources
+ * https://zaccharles.medium.com/calculating-a-dynamodb-items-size-and-consumed-capacity-d1728942eb7c
+ * https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/CapacityUnitCalculations.html
+ */
+ val dynamoFixedByteSize =
Review Comment:
We'll adapt when we rebase 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] Protect against doing requests for snapshots that are over 400KB. [incubator-pekko-persistence-dynamodb]
Posted by "spangaer (via GitHub)" <gi...@apache.org>.
spangaer commented on PR #13:
URL: https://github.com/apache/incubator-pekko-persistence-dynamodb/pull/13#issuecomment-1862318098
The headers should be fixed, but it looks like we're still doing something wrong with the formatting.
--
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-dynamodb] coreyoconnor commented on pull request #13: Protect against doing requests for snapshots that are over 400KB.
Posted by "coreyoconnor (via GitHub)" <gi...@apache.org>.
coreyoconnor commented on PR #13:
URL: https://github.com/apache/incubator-pekko-persistence-dynamodb/pull/13#issuecomment-1442699998
Nice!
To clarify: This is to improve the error handling in the case a snapshot would be rejected due to exceeding DynamoDB's maximum item size?
I also like that this is part of the code that would be required to dispatch large snapshots to S3 :)
--
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-dynamodb] spangaer commented on pull request #13: Protect against doing requests for snapshots that are over 400KB.
Posted by "spangaer (via GitHub)" <gi...@apache.org>.
spangaer commented on PR #13:
URL: https://github.com/apache/incubator-pekko-persistence-dynamodb/pull/13#issuecomment-1517568213
> To clarify: This is to improve the error handling in the case a snapshot would be rejected due to exceeding DynamoDB's maximum item size?
It is, avoiding a sizeable round trip in doing so.
--
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-dynamodb] mdedetrich commented on a diff in pull request #13: Protect against doing requests for snapshots that are over 400KB.
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #13:
URL: https://github.com/apache/incubator-pekko-persistence-dynamodb/pull/13#discussion_r1117456706
##########
src/main/scala/akka/persistence/dynamodb/snapshot/package.scala:
##########
@@ -21,4 +21,21 @@ package object snapshot {
val Payload = "pay"
val SequenceNr = "seq"
+ /**
+ * Returns (a slightly overestimated) size of the fixed fields in a snapshot DynamoDB record.
+ *
+ * Assumes the maximum of 21 bytes for a number.
+ *
+ * Sources
+ * https://zaccharles.medium.com/calculating-a-dynamodb-items-size-and-consumed-capacity-d1728942eb7c
+ * https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/CapacityUnitCalculations.html
+ */
+ val dynamoFixedByteSize =
Review Comment:
Should be `DynamoFixedByteSize` since its a constant (Scala convention).
--
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] Protect against doing requests for snapshots that are over 400KB. [incubator-pekko-persistence-dynamodb]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #13:
URL: https://github.com/apache/incubator-pekko-persistence-dynamodb/pull/13#issuecomment-1862328516
@spangaer SnapshotTooBigSpec has 2 formatting issues
* incorrect indent on line 31
* stray space at end of line 61
--
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] Protect against doing requests for snapshots that are over 400KB. [incubator-pekko-persistence-dynamodb]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #13:
URL: https://github.com/apache/incubator-pekko-persistence-dynamodb/pull/13#issuecomment-1883065447
> Thanks, just found a couple of things.
>
> Also it would be ideal if we could write a test for this.
@mdedetrich could you re-review? I've created a 1.0.x branch so it should now be safe to merge 1.1 stuff.
--
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