feature/remove-migration-states #1

tomchkk · 2023-02-03T17:10:22Z

This PR removes the migration state handling initially included, modifying the initial MutexRelay to use Laravel's cache locking as opposed to the more complex MutexQueue implementation.

kubatek94

Lovely changes Tom, negative code is the best :D

I have added a few small suggestions.

However I think my main concern is about the case when the '--mutex' option is not used, and we would like to fall back to standard Laravel migration command behaviour.

I appreciate the use of the Null design pattern and it seems like a sound design, however I am not sure if simply short circuiting and calling the parent handle method wouldn't be more appropriate. This would result in more closer behaviour to the original (however it may end up working in the future Laravel versions) and would truly mean that we don't really 'activate' our behaviour until that '--mutex' option is passed.

I also wonder about the handling of process signals in the original Migrate command, vs our extension without '--mutex' option and whether it will leave our extension truly 'inactive'.

Just some food for thought :)

README.md

kubatek94 · 2023-02-15T09:54:42Z

src/MigrateCommandExtension.php

+        $this->processor->terminate();
    }

    private function extendSignature(): void


I can't shake the feeling, that while it may include a bit of duplication of the option name, it's much easier to mentally parse something like this:

$this->signature = "$this->signature { --mutex : Run a mutually exclusive migration }";

I ported this from some work I'd done extending migrations on UK Bathrooms. I looked into it a bit further and think I've found something of a compromise. See what you think.

kubatek94 · 2023-02-15T10:04:02Z

src/Mutex/MutexRelay.php

+
+    public function __construct(
+        private readonly Repository $cache,
+        private readonly int $ttl_seconds = 60


I think we would normally go with camel case, but I would also consider a bit more descriptive name:

Suggested change

private readonly int $ttl_seconds = 60

private readonly int $lockDurationSeconds = 60

Agreed, not sure how that ended up like that 😊

kubatek94 · 2023-02-15T10:06:56Z

src/Mutex/MutexRelay.php

            return false;
        }

        return Str::contains($th->getMessage(), ['Base table or view new found', 'cache'], true);


I think a more robust way would be to check the SQL error code - I think there should be a specific one for this error accessible somewhere in the exception properties.

I've updated it, though it did require me to add an extended exception class, just so I could set a error code as a string. Still, however, I found that I needed to check for some text in the message, to be sure of the table name in question.

kubatek94 · 2023-02-15T10:10:00Z

src/Mutex/MutexRelay.php

+        $provider = $this->cache->getStore();

-        return $this->queue->contains(fn ($item) => $item === $meta);
+        return $this->lock = $provider->lock(self::KEY . '.lock');


This is not a big deal, because in the current form this scenario won't happen, however normally I think the first thing a method like that should do is check if $this->lock is already set, and return that instead, only assigning the lock property once. I would have an expectation like this, because the method assigns a single shared class property, rather than just returning $provider->lock() result.

100% - I'd definitely code it like that ordinarily and think I didn't for the precise reason that it seemed like it wouldn't occur. Anyway, I added an early return if the instance already exists.

kubatek94 · 2023-02-15T10:18:16Z

src/ServiceProvider.php

                Cache::store(Config::get('mutex-migrations.queue.store')),
                Config::get('mutex-migrations.queue.ttl_seconds')


I would consider renaming the config properties a little to fit with the changes

Suggested change

Cache::store(Config::get('mutex-migrations.queue.store')),

Config::get('mutex-migrations.queue.ttl_seconds')

Cache::store(Config::get('mutex-migrations.lock.store')),

Config::get('mutex-migrations.lock.ttl_seconds')

src/MigrateCommandExtension.php

tomchkk · 2023-02-17T15:22:53Z

Lovely changes Tom, negative code is the best :D

I have added a few small suggestions.

However I think my main concern is about the case when the '--mutex' option is not used, and we would like to fall back to standard Laravel migration command behaviour.

I appreciate the use of the Null design pattern and it seems like a sound design, however I am not sure if simply short circuiting and calling the parent handle method wouldn't be more appropriate. This would result in more closer behaviour to the original (however it may end up working in the future Laravel versions) and would truly mean that we don't really 'activate' our behaviour until that '--mutex' option is passed.

I also wonder about the handling of process signals in the original Migrate command, vs our extension without '--mutex' option and whether it will leave our extension truly 'inactive'.

Just some food for thought :)

Thanks for the useful suggestions and comments. I've made updates based on the specific suggestions you made in order to check them off the list, as it were.

Re: keeping the two paths completely separate - I get what you're saying and have been looking into it, but I haven't immediately managed to find a satisfactory way of achieving that - at least while preserving the --mutex option. I'll have a think about it over the next week and have another look next Friday.

tomchkk · 2023-02-20T09:22:05Z

OK - I figured out a workable solution. You'll notice I left in (one of) the migration processing elements. I did attempt to move that into the new MutexMigrationCommand class itself, but I didn't really like the result or the prospect of having to test the process within the command. I also had to update the signal handling as it wasn't working and the docs have been changed since the original work. See what you think.

kubatek94

I'm really happy with the changes @tomchkk - I think it's a pretty good solution in the end, which keeps the original implementation pretty much unchanged. Please take a look at further feedback that I think would be good to consider to make it more bullet proof.

kubatek94 · 2023-02-20T11:49:12Z

src/Mutex/MutexRelay.php

-        sleep($wait++);
-
-        return $wait;
+        return $th->getCode() === '42S02' && Str::contains($th->getMessage(), 'cache_locks');


I have been thinking about it, and how it would be good for the table name to not be hardcoded, but instead use whatever the lock provider implementation is using. However, I started thinking, do we really need to do that?

Given that the throwable instance here is QueryException, we know that the lock implementation is backed with 'DatabaseStore', we know it failed to run a query, we know it failed when we tried to get a lock, and we know it failed because 'a table or view not found' - I think it's safe to assume it's due to the locks table not existing.

However, if you think searching for the table name is preferable, then I do believe we should read that table name from the config of the cache store, named by the 'mutex-migrations.lock.store'. That's because I see that in the source code of DatabaseStore the table name has default value set to 'cache_locks', but for example in GRJ codebase, the 'cache.stores.database.table' config value is 'cache' - and that's what would be used.

OK - totally got both points about a) not hardcoding the lock table name and b) whether we really need this check. I tried without it and felt that the result could be confusing because you're basically just told that you probably need to run migrations. It misses the specificity of the situation.

So I'd say we keep this and, to that end, I've updated it so that it tries to fetch the configured lock_table value, or uses a default.

kubatek94 · 2023-02-20T11:57:49Z

src/MutexMigrateCommand.php

+        } catch (\Throwable $th) {
+            $this->components->error($th->getMessage());
+
+            return self::FAILURE;


I think I would only handle specific exceptions this way (just printing their message). The behaviour of the base migrate command is to not catch them, so that they will end up within sentry and printed out to console - including all the detail which may be helpful sometimes. If e.g. you think handling the 'cache table not found' would be better like this, then perhaps we should catch that specific exception only, and let the rest fail as usual?

Yes, good point. I've updated it to only handle the now expected cache lock table not found exception.

kubatek94 · 2023-02-20T12:04:58Z

tests/Unit/Mutex/fixtures/TestPDOException.php

@@ -0,0 +1,14 @@
+<?php
+
+namespace Netsells\LaravelMutexMigrations\Tests\Unit\Mutex\fixtures;


I think following normal namespace conventions would be good here as well.

Suggested change

namespace Netsells\LaravelMutexMigrations\Tests\Unit\Mutex\fixtures;

namespace Netsells\LaravelMutexMigrations\Tests\Unit\Mutex\Fixtures;

tomchkk · 2023-02-21T10:22:42Z

Thanks for the feedback @kubatek94 - will try to look on Friday, but feels like we're getting close now 😊

Btw, I learned about isolatable commands for the first time yesterday. I'm fairly sure this is a relatively recent addition to the documentation. Still my heart sank for a moment after spending time on this solution. However, looking at it - and the code - all it seems to do is to add a lock to prevent a command running elsewhere whereas this package will effectively allow migrations to be queued up. Thoughts?

kubatek94 · 2023-02-22T16:03:18Z

Hey @tomchkk, yeah we are definitely close - it's just few new things I picked up on, but nothing major. Thanks for sharing about isolatable commands - today I learnt :D As you say, it seems that they have different behaviour than what we wanted to achieve here. We wanted to block our command until the other is done, whereas the isolatable interface would cause our migration command to 'succeed' straightaway if another instance is running, so not desirable. Which is great, as I had the same feeling you describe for a moment there haha.

kubatek94

Well done @tomchkk, great changes!

tomchkk added 2 commits February 3, 2023 16:37

Replace migration state modes with simpler Laravel cache locking

b663b3c

Update README

f37170d

tomchkk requested a review from kubatek94 February 3, 2023 17:10

kubatek94 suggested changes Feb 15, 2023

View reviewed changes

tomchkk added 4 commits February 17, 2023 14:28

Simplify modification of command signature

95c06d0

Improve parameter naming

6e64b17

Ensure getLock returns any existing lock

7d477c8

Add SQLSTATE code check to determine exception

874a6dc

tomchkk added 2 commits February 19, 2023 11:40

Separate handling of mutex migrations from standard migrations

9f51c0d

Remove StandardMigrationProcessor

ce2f2ab

tomchkk requested a review from kubatek94 February 20, 2023 09:22

kubatek94 suggested changes Feb 20, 2023

View reviewed changes

tomchkk added 7 commits February 24, 2023 19:51

Enable cache lock table name injection

1d2a576

Modify command to only handle a specific execption

54208f0

Correct namespace

5848bd7

Set default lock table value as const

ffd0f02

Include additional output when locks not acquired/released

513dd28

Add test workflow

31ec1f8

Correctly move case-adjusted file path

70a6f0a

tomchkk requested a review from kubatek94 March 3, 2023 15:03

kubatek94 approved these changes Mar 6, 2023

View reviewed changes

tomchkk merged commit eae7bde into main Mar 9, 2023

tomchkk deleted the feature/remove-migration-states branch March 9, 2023 15:39

	private readonly int $ttl_seconds = 60
	private readonly int $lockDurationSeconds = 60

		Cache::store(Config::get('mutex-migrations.queue.store')),
		Config::get('mutex-migrations.queue.ttl_seconds')

		@@ -0,0 +1,14 @@
		<?php

		namespace Netsells\LaravelMutexMigrations\Tests\Unit\Mutex\fixtures;

feature/remove-migration-states #1

feature/remove-migration-states #1

Uh oh!

Conversation

tomchkk commented Feb 3, 2023

Uh oh!

kubatek94 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomchkk commented Feb 17, 2023

Uh oh!

tomchkk commented Feb 20, 2023

Uh oh!

kubatek94 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomchkk commented Feb 21, 2023

Uh oh!

kubatek94 commented Feb 22, 2023

Uh oh!

kubatek94 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants