Skip to content
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

docs: move EAN8 product directories #363

Merged
merged 5 commits into from
Oct 8, 2024
Merged

docs: move EAN8 product directories #363

merged 5 commits into from
Oct 8, 2024

Conversation

stephanegigandet
Copy link
Contributor

@stephanegigandet stephanegigandet commented Jun 21, 2024

### Products with changed codes update

Need to:
- remove old code from MongoDB
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a script for that openfoodfacts/openfoodfacts-server#7249

Need to:
- remove old code from MongoDB
- notify REDIS of removal of old code
- update new code in .sto file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will you also update old versions .sto ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not planning to.


## Moving products

We need to move products on OFF, OBF, OPF and OPFF roughly at the same time, in order to avoid issues if products are moved from one flavor to another that uses a different barcode to path mapping.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means you have to change the code of those projects too ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Or we move them first to the new code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your plan for the migration ? (better write it down to follow it the D day).

As I understand we first deploy the new product opener version.

Then we run the script.

I would personally prefer a safe path of running the script with ProductOpener down (ideally it could be up in read only mode, but I don't think we have this feature ;-) ).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also if we do it with ProductOpener down, don't forget to also stop the various services that might attend to it (generate_feeds_*, minion, cloud_vision).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking I can make it transparent: if the old path exists, Product Opener uses it, otherwise it uses the new path. That way products can be moved at any time. And once the migration is 100% complete, I remove the check for the old path.

@stephanegigandet stephanegigandet changed the title doc: move EAN8 product directories docs: move EAN8 product directories Jun 25, 2024
Comment on lines +138 to +144
When we update one flavor, the new normalization and path generation code will be live, but the products will take time to migrate. In the mean time, those products will appear to be non-existing if someone tries to access their product page (or call the API).

One option could be to stop the service while we run the migration script (but it might take hours, especially on OFF where we need to move 250k products.

Or we could change the code to make it work with both old paths and new paths.

DECISION: change the Product Opener code (split_code function) so that if the old normalized path exist (which means the product has not been migrated), we use it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another strategy that I can think of:

  • deploy a code that use new code normalization for computing path for new products, but is able to fallback on the old one (if the normalized path does not exists, try the non normalized one)
  • run the script when you want
  • deploy a code with support only for normalized code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In effect, that's what we do. We check if the old path exists and use it if it does, we use it. It has the same effect as checking first if the new path exists. (unless if for some reason both paths exist..)

@teolemon
Copy link
Member

teolemon commented Oct 4, 2024

Does this mean we have to update our path building logic for the Dart packages (and others) if we have any @stephanegigandet @monsieurtanuki

@monsieurtanuki
Copy link

Does this mean we have to update our path building logic for the Dart packages (and others) if we have any @stephanegigandet @monsieurtanuki

@teolemon In dart we do compute the barcode subfolder for all the product images:

  /// Returns the barcode sub-folder (without trailing '/').
  ///
  /// For instance:
  /// * `12345678` for barcode `12345678`
  /// * `123/456/789` for barcode `123456789`
  /// * `123/456/789/0` for barcode `1234567890`
  /// * `123/456/789/0123` for barcode `1234567890123`
  static String getBarcodeSubPath(final String barcode) {

The first step would be to give ourselves more flexibility, like static String getBarcodeSubPath(String barcode, bool newEAN8mode).

Then everywhere in Smoothie when we display an image, use the "old" EAN8 syntax and the "new" 00000 syntax as a fallback. We don't store image file paths, we compute them on demand.

I've double-checked, that could have been problematic in Smoothie when we write (e.g. crop) an image as our methods currently compute and return the file path. But eventually we don't use those paths: as we work on background tasks, all we care about is if the task failed or not, and the changed data will be provided by the refreshed data of the product.

That will impact only the EAN8, of course.

In Smoothie we can start now to support the old then the new syntax as fallback.

@stephanegigandet
Copy link
Contributor Author

@monsieurtanuki @teolemon

the server still support the old paths, but yes we should update the logic in the Dart package.

/// * 12345678 for barcode 12345678
/// * 123/456/789 for barcode 123456789

That will impact only the EAN8, of course.

Also others, e.g. 9 digit 123456789 is now in 000/012/345/6789. We not only split the EAN8, when we compute paths, we pad all codes with 0s so that they have 13 digits.

Copy link
Member

@alexgarel alexgarel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kudos for this though work !

@stephanegigandet stephanegigandet merged commit b12a3fd into develop Oct 8, 2024
5 checks passed
stephanegigandet added a commit to openfoodfacts/openfoodfacts-server that referenced this pull request Oct 8, 2024
… DO NOT MERGE (#10472)

I closed the very old PR
#3915 and I am
opening this new one to put it on the first page.

Original issue:
#3818

We will need to move the file structures on all flavors (OFF, OBF, OPF,
OPFF) roughly at the same time, as otherwise we will have issues if some
products are moved from one flavor to another.

So the migration script is using "old" conventions so that it can run on
the old obf / opf / opff code (without needing newer modules).

The migration script is first used to assess the situation (how many
products would be moved, how many products have conflicts etc.) on all
flavors.

PR in infrastructure repo to detail the migration:
openfoodfacts/openfoodfacts-infrastructure#363

---------

Co-authored-by: hangy <[email protected]>
Co-authored-by: Alex Garel <[email protected]>
Co-authored-by: OFF <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Store EAN-8 products in /products/000/001/234/5678
4 participants