Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Lock] DynamoDB store#60138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
natepage wants to merge33 commits intosymfony:7.4
base:7.4
Choose a base branch
Loading
fromnatepage:feature/lock-dynamodb-store

Conversation

natepage
Copy link
Contributor

QA
Branch?7.3
Bug fix?no
New feature?yes
Deprecations?no
IssuesFix#59996
LicenseMIT

This PR introduces a new Lock store using AWS DynamoDb. The idea is coming from thisarticle.

The implementation is heavily based on:

valtzu and GromNaN reacted with rocket emoji
@carsonbot

This comment has been minimized.

@natepagenatepage marked this pull request as ready for reviewApril 4, 2025 05:03
@carsonbotcarsonbot added this to the7.3 milestoneApr 4, 2025
@carsonbotcarsonbot changed the title[Lock][WIP] Initial pass on DynamoDb lock store[Lock] [WIP] Initial pass on DynamoDb lock storeApr 4, 2025
@natepagenatepage changed the title[Lock] [WIP] Initial pass on DynamoDb lock store[Lock] DynamoDb storeApr 4, 2025
@stof
Copy link
Member

stof commentedApr 4, 2025

this new store should probably in a bridge package, so that we can manage its dependency properly.

GromNaN reacted with thumbs up emoji

@OskarStarkOskarStark changed the title[Lock] DynamoDb store[Lock] DynamoDB storeApr 7, 2025
@natepage
Copy link
ContributorAuthor

@stof Thank you for your feedback, I did move it to its own bridge, I'm not sure if I've done everything required from a structural POV.

I still didn't touch the tests yet

@OskarStark
Copy link
Contributor

this new store should probably in a bridge package, so that we can manage its dependency properly.

While I agree on that@stof, right now there is no Bridge namespace:
CleanShot 2025-04-09 at 12 14 02@2x

@stof
Copy link
Member

stof commentedApr 9, 2025

@OskarStark this does not prevent us fromadding it if we need it.

GromNaN reacted with thumbs up emoji

@natepage
Copy link
ContributorAuthor

@stof@OskarStark thank you for taking the time to review this 😄

I've actually updated the PR to add this Bridge namespace following the structure from the Messenger SQS example, but the package validation is failing and I'm unsure why as I've placed the composer.json file exactly in the same level as the Messenger SQS one, trying to investigate that right now

@natepage
Copy link
ContributorAuthor

Yay! Package checks are passing, the match case forcomponent_bridge wasn't behaving as expected, back onto PHP tests 😄

@OskarStark
Copy link
Contributor

Yes, we need to keep in mind, that we need to configure a proper split for the repo etc.

@natepage
Copy link
ContributorAuthor

@stof@OskarStark

The current state of this is:

  • Moved the logic into its own component bridge
  • Implemented integration tests by using localstack in CI to have a local DynamoDB instance
  • Updated the default options logic based on feedback
  • Implemented unit tests for the store constructor logic (DSN/Options)

I have re-requested your reviews, thank you 😄

GromNaN reacted with thumbs up emoji

Copy link
Member

@GromNaNGromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Just a few more adjustments for this new package, and it looks good to me. I haven't read over the implementation in detail.

Copy link
Member

@GromNaNGromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

SGTM.
Just a line of code that can be removed.

Comment on lines +110 to +114
if ('default' !== ($params['host'] ?? 'default')) {
$clientConfiguration['endpoint'] = \sprintf('%s://%s%s', ($options['sslmode'] ?? null) === 'disable' ? 'http' : 'https', $params['host'], ($params['port'] ?? null) ? ':'.$params['port'] : '');
if (preg_match(';^dynamodb\.([^\.]++)\.amazonaws\.com$;', $params['host'], $matches)) {
$clientConfiguration['region'] = $matches[1];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I don't see where this code is tested. Can you add a test case with a custom host?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@GromNaN Thistest is settinghost => localhost which tests that part

@natepage
Copy link
ContributorAuthor

Hi there! 👋
I'm sure everybody is quite busy prepping for 7.3 release, but just a gentle follow up on this one,
I would like to know if anything else is needed from me so I can organise myself to have time to spend on this, thank you 😄

@fabpotfabpot modified the milestones:7.3,7.4May 26, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@GromNaNGromNaNGromNaN approved these changes

@jderussejderusseAwaiting requested review from jderussejderusse is a code owner

@stofstofAwaiting requested review from stof

@OskarStarkOskarStarkAwaiting requested review from OskarStark

Assignees
No one assigned
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.

[Lock] DynamoDB store
6 participants
@natepage@carsonbot@stof@OskarStark@GromNaN@fabpot
[8]ページ先頭
©2009-2025 Movatter.jp