pkp/pkp-lib11758 fix region codes #11899
Conversation
…ns that needs to be fixed, ensure the jobs can be re-tried
|
The job RegionMappingTmpClear is maybe not needed, because the table is cleared before insertion in RegionMappingTmpInsert, but I left it for now. Please let me know what you think, if I should remove it... |
| Schema::table('metrics_submission_geo_daily', function (Blueprint $table) { | ||
| $table->string('region', 3)->change(); | ||
| }); | ||
| Schema::table('metrics_submission_geo_monthly', function (Blueprint $table) { | ||
| $table->string('region', 3)->change(); | ||
| }); |
There was a problem hiding this comment.
I think jobs shouldn't touch the structure of the database.
| DB::table('metrics_submission_geo_daily')->whereNotNull('region')->exists()) { | ||
| if (DB::table('metrics_submission_geo_monthly')->where('region', '<>', '')->exists() || | ||
| DB::table('metrics_submission_geo_daily')->where('region', '<>', '')->exists()) { | ||
|
|
There was a problem hiding this comment.
GitHub doesn't allow me to comment on the lines below, but adding an index to the temporary table's columns might be helpful to performance.
| Schema::table('metrics_submission_geo_daily', function (Blueprint $table) { | ||
| $table->string('region', 7)->change(); | ||
| }); | ||
| Schema::table('metrics_submission_geo_monthly', function (Blueprint $table) { | ||
| $table->string('region', 7)->change(); | ||
| }); |
There was a problem hiding this comment.
This sounds risky, if the job never gets completed, the database will be left with an unexpected structure.
The same for the indexes below.
| } | ||
| $jobs[] = new CleanTmpChangesForRegionCodesFixes(); | ||
| Bus::chain($jobs) | ||
| ->catch(function (Throwable $e) { |
There was a problem hiding this comment.
There should be some error logging here, the Throwable covers even syntax errors at included files.
| * Copyright (c) 2022-2025 Simon Fraser University | ||
| * Copyright (c) 2022-2025 John Willinsky |
There was a problem hiding this comment.
For new files we're keeping just the initial year
| * Copyright (c) 2022-2025 Simon Fraser University | |
| * Copyright (c) 2022-2025 John Willinsky | |
| * Copyright (c) 2025 Simon Fraser University | |
| * Copyright (c) 2025 John Willinsky |
| namespace PKP\migration\upgrade\v3_4_0\jobs; | ||
|
|
||
| use Illuminate\Support\Facades\DB; | ||
| use PKP\jobs\BaseJob; | ||
|
|
||
| class RegionMappingTmpClear extends BaseJob | ||
| { | ||
| /** | ||
| * Execute the job. | ||
| */ | ||
| public function handle(): void | ||
| { | ||
| // clear region_mapping_tmp table | ||
| DB::table('region_mapping_tmp')->delete(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This cleanup is covered by the RegionMappingTmpInsert.php, so this whole file can be removed.
| DB::table('region_mapping_tmp')->insert([ | ||
| 'country' => $this->country, | ||
| 'fips' => $fips, | ||
| 'iso' => $iso | ||
| ]); |
There was a problem hiding this comment.
You can store the records in an array and do a batch insert (chunks of 1000).
| /** The range of metrics_submission_geo_daily_ids to consider for this update */ | ||
| protected int $startId; | ||
| protected int $endId; | ||
|
|
||
| /** | ||
| * Create a new job instance. | ||
| */ | ||
| public function __construct(int $startId, int $endId) | ||
| { | ||
| parent::__construct(); | ||
| $this->startId = $startId; | ||
| $this->endId = $endId; | ||
| } |
There was a problem hiding this comment.
As this isn't going to be extended, it can be private and it's possible to simplify the initialization
| /** The range of metrics_submission_geo_daily_ids to consider for this update */ | |
| protected int $startId; | |
| protected int $endId; | |
| /** | |
| * Create a new job instance. | |
| */ | |
| public function __construct(int $startId, int $endId) | |
| { | |
| parent::__construct(); | |
| $this->startId = $startId; | |
| $this->endId = $endId; | |
| } | |
| /** | |
| * Create a new job instance. | |
| * $startId and $endId provide the range of metrics_submission_geo_daily_ids to consider for this update | |
| */ | |
| public function __construct(private int $startId, private int $endId) | |
| { | |
| parent::__construct(); | |
| } |
| // update region code from FIPS to ISP, according to the entries in the table region_mapping_tmp | ||
| // Laravel join+update does not work well with PostgreSQL, so use the direct SQLs | ||
| // daily | ||
| if (substr(Config::getVar('database', 'driver'), 0, strlen('postgres')) === 'postgres') { |
There was a problem hiding this comment.
I think it should be possible to build this update and the others using the Laravel query builder.
2 participants
s. #11758