Introduce new batch update for settings reduces the number of queries sent to the database and redis #213
Conversation
AhmedAli-Qawafel
changed the title
|
@rubenvanassche any update on this? |
src/SettingsMapper.php
Outdated
| $propertiesToUpdate = $properties | ||
| ->reject(fn ($payload, string $name) => $config->isLocked($name)); | ||
|
|
||
| (clone $propertiesToUpdate) |
There was a problem hiding this comment.
Why would you add a clone here?
There was a problem hiding this comment.
You're correct that there's no need to use clone, as reject returns a new filtered collection.
src/SettingsMapper.php
Outdated
| 'payload' => $payload, | ||
| ]; | ||
| }) | ||
| ->groupBy('group') |
There was a problem hiding this comment.
Is this required, we're saving one settings class so we'll always have one group right?
There was a problem hiding this comment.
You're correct that there's no need to group, since it will always have one group.
| })->toArray(); | ||
|
|
||
| try { | ||
| $this->getBuilder() |
There was a problem hiding this comment.
Let us let it fail hard, no need to catch this.
| @@ -71,6 +73,25 @@ public function updatePropertyPayload(string $group, string $name, $value): void | |||
| ]); | |||
| } | |||
|
|
|||
| public function updatePropertiesPayload(string $group, Collection $properties): void | |||
There was a problem hiding this comment.
Since this will be a major release, could we drop updatePropertyPayload altogether to make a unified update path in the code?
| @@ -71,6 +73,25 @@ public function updatePropertyPayload(string $group, string $name, $value): void | |||
| ]); | |||
| } | |||
|
|
|||
| public function updatePropertiesPayload(string $group, Collection $properties): void | |||
| { | |||
| $propertiesInBatch = (clone $properties)->map(function ($property) { | |||
There was a problem hiding this comment.
Let's drop the clone here
| { | ||
| $data = []; | ||
|
|
||
| $properties->each(function (array $property) use ($group, &$data) { |
There was a problem hiding this comment.
Can't we rewrite this as a mapWithKeys?
| $table->json('payload'); | ||
|
|
||
| $table->timestamps(); | ||
|
|
||
| $table->unique(['group', 'name']); |
… using only one group
|
"Hi @rubenvanassche , I've made the changes you suggested in my code and I wanted to ask if you could take another look at it when you have a chance. I'd really appreciate your feedback on the changes I've made. Thank you |
|
Thanks! |
|
Hey, on the contrary we have added a default value for that field to be false on this pull request so it doesn't matter if it's not sent to the database it already have a default value we migrated our live project and it's working fine with the new updates kindly send me a trace or the called method please |
|
I just realized I hadn't published the new migration after upgrading to version 3. Thanks for the quick response. |
|
Pleasure to help 🙏 |
The purpose of this pull request is to improve the performance of database updates discussed here, by grouping records by their respective groups and performing batch updates on each group. Instead of saving each record in a single query, this approach sends a single request per group, resulting in faster and more efficient updates at the database level.
It is safe to use this feature because it includes error handling that falls back to the original save logic in the event of any issues. This ensures that data is not lost and that the application can continue to function correctly even if an error occurs during the save process.