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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move queued tracking lock implementation to core for other uses. #14686
Conversation
|
||
$tableName = self::getTableName(); | ||
|
||
DbHelper::createTable($tableName, " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the table is installed on demand... If there's a race condition, should we maybe catch a "table already exists" error and fail silently? Or we just install the table the regular way and provide an update for it as well? IT be fine if table is not needed by most users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the SQL was 'CREATE IF NOT EXISTS', but I'll double check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's not CREATE IF NOT EXISTS
, but the exception is already caught: https://github.com/matomo-org/matomo/blob/3.x-dev/core/Db/Schema/Mysql.php#L452
use Piwik\Db; | ||
use Piwik\DbHelper; | ||
|
||
class MySqlLockBackend implements LockBackend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to write some simple tests for this backend as well? Can be probably copied from Queued tracking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I forgot about the backend, I'll add the tests.
@tsteur Updated, though can't be merged just yet. I moved the table creation to Schema.php, since the lock could be used during tracking, and |
No description provided.