metacpan-api icon indicating copy to clipboard operation
metacpan-api copied to clipboard

Verify Indices Mappings and Clear all Indices

Open bodo-hugo-barwich opened this issue 3 years ago • 6 comments

This implements the functionality discussed at: Fix Corrupted Indices and Verify Mappings It is also able to fix difficulties and corruptions as documented at Unexpected Index Corruption

Additionally it implements Exit with Error Code for unconfirmed Operations.

To test all the functionality it features several Mapping Script Tests.

bodo-hugo-barwich avatar Jul 30 '22 15:07 bodo-hugo-barwich

This is a big (adding 700 lines) change that introduces valuable functionality. My only observation is it seems splittable conceptually into two pieces of functionality; the verify, and the clear-indices. Would it make sense to make a separate PR that only introduced the checking/verification code from this? That would be a smaller, much less risky change; then this one could be rebased past that, with the higher-risk code isolated and more easily reviewable.

mohawk2 avatar Aug 13 '22 22:08 mohawk2

Yes, you are right. I also noticed that I actually was working on two different tasks. But unfortunately both share the same code and also the same tests. So I need to rebuild a separate branch from the master branch and reimport the sub routines from the current branch, and review it of course. Also the second branch would need to be rebased according to which branch is merged first.

bodo-hugo-barwich avatar Aug 20 '22 10:08 bodo-hugo-barwich

I built a new branch at: Clear Unknown Indices which implements the --delete --all option and also introduces important tests which are used for the mappings verification tests.

bodo-hugo-barwich avatar Aug 20 '22 12:08 bodo-hugo-barwich

I would really like to see the verify done (including merged and released, since it is inherently less risky) first, not the clear. Is there a good reason why it can't be done in that order?

mohawk2 avatar Aug 20 '22 22:08 mohawk2

Actually the verify mappings logic is the bigger part of this development with the introducing the _compare_mapping(), _build_mapping() and _build_aliases() methods with more than 200 lines of code additionally to the tests with another 100 lines of code. The tests of the verify mappings logic require tests of --create and --update index functionality which require some additional 100 lines of changes. So my idea was to split the code more evenly and already introduce tests in the smaller task that will thin out the verify mappings development which supposedly would increase the maintainability and reviewabilty of the development.

On the other side the tests of the --delete --all logic show that this functionality is save and should not be able to run in production if my assumptions about the environment recognition are as documented in the related issue: recognize development environment flag development environment with environment variables Still there is also a manual protection as requested at: manual confirmation required

bodo-hugo-barwich avatar Aug 21 '22 10:08 bodo-hugo-barwich

I rebuilt the "Verify Mappings" functionality at: Verify Indices Mappings which is the semantical split of this development.

bodo-hugo-barwich avatar Aug 30 '22 08:08 bodo-hugo-barwich