Conversation
…ion-sdk-dynamodb into ajewell/buckets
|
@ajewellamz and @mahnushm, I noticed you are updating the smithy model files. |
|
@ajewellamz and @mahnushm, I noticed you are updating the smithy model files. |
|
@ajewellamz and @mahnushm, I noticed you are updating the smithy model files. |
|
@ajewellamz and @mahnushm, I noticed you are updating the smithy model files. |
…ion-sdk-dynamodb into ajewell/buckets
rishav-karanjit
left a comment
rishav-karanjit
left a comment
There was a problem hiding this comment.
I have reviewed everything under specification and testVectors. There is one blocking comment in spec. Remaining are suggestions but not blocking.
I will review rest the next time.
| and then constrain the number of partitions for the other two beacons. | ||
|
|
||
| The partition used to calculate the hash for a constrained beacon is | ||
| `ItemsPartition % BeaconConstraint` where `%` is the `modulo` or `remainder` operation. |
There was a problem hiding this comment.
Question: is ItemsPartition, the partition number assigned to the whole table?
There was a problem hiding this comment.
Answering my own question: ItemsPartition is assigned to each item which is a random number between 0 to max partition. Correct me if I am understanding wrong
specification/changes/2025-08-25-beacon-partitions/background.md
Outdated
Show resolved
Hide resolved
| then you will immediately need to start making 7 queries, | ||
| but the sixth and seventh queries will return a comparatively small number of items. | ||
|
|
||
| ### Beacon Constraints |
There was a problem hiding this comment.
Question: this should be optional right?
|
|
||
| - An integer in the range 0..254 | ||
| - The number of the partition currently under consideration in some context | ||
| - For customers not using PartitionBeacons, this is always `1` |
There was a problem hiding this comment.
Blocking: Shouldn't this be 0?
There was a problem hiding this comment.
Currently, there are lots of test which is great. In addition to current test, some tests to take into consideration are:
- I don't see any test using defaultNumberOfPartitions. So, we might want to check defaultNumberOfPartitions Behavior. Something like:
- set defaultNumberOfPartitions to 3
- Beacon A: no numberOfPartitions specified, uses 3
- Beacon B: numberOfPartitions specified, uses the specific numberOfPartitions
- Currently, I see the maximum number of partitions is only 5. I am not too worried when it comes to integer but in addition to current test we at-least might want to test when max number of partitions is 1 which will reflect number of partitions before beacon partition was introduced.
- Currently, we query only already existing values. It should be worth it to add a test that queries non existed value and see what it returns
|
@ajewellamz and @rishav-karanjit, I noticed you are updating the smithy model files. |
Co-authored-by: Rishav karanjit <karanjitrishav4@gmail.com>
Co-authored-by: Rishav karanjit <karanjitrishav4@gmail.com>
Co-authored-by: Rishav karanjit <karanjitrishav4@gmail.com>
Co-authored-by: Rishav karanjit <karanjitrishav4@gmail.com>
5 participants
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.