Skip to content

Load metadata from file#3185

Open
dponomarenko-firstorion wants to merge 6 commits into
google:masterfrom
dponomarenko-firstorion:load-metadata-from-file
Open

Load metadata from file#3185
dponomarenko-firstorion wants to merge 6 commits into
google:masterfrom
dponomarenko-firstorion:load-metadata-from-file

Conversation

@dponomarenko-firstorion

@dponomarenko-firstorion dponomarenko-firstorion commented Aug 21, 2023

Copy link
Copy Markdown

I would like to propose to add one more metadata loading mechanism for libphonenumber cpp version , which would load metadata from protobuf binary file.

This functionality is desired by developers, as it eliminates the need to rebuild and release new libphonenumber version when only data update is required.

I am proposing to add compile-time switch to differentiate between data loading from binary array and data loading from protobuf binary file.

Also new public function “bool ReloadMetadata(const string& filepath) added to PhoneNumberUtil class intended to provide ability to reload metadata without restarting client application process. Here is what ReloadMetadata will do :

  1. free the memory occupied by already loaded metadata
  2. load new metadata into the memory
  3. replace protobuf binary file with new one.

@kkeshava @katbohm

@dponomarenko-firstorion dponomarenko-firstorion requested a review from a team as a code owner August 21, 2023 16:29
@tvislavski tvislavski requested a review from katbohm November 15, 2023 11:04

@katbohm katbohm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi there!
Thank you for your pull request and contributing to the LibPhoneNumber library.
As for the proposed changes, I think the functionality might be useful. My main note is that I think the two binary *.dat files shouldn't be checked in, but rather generated by build rules.

Comment thread cpp/CMakeLists.txt Outdated
Comment thread cpp/src/phonenumbers/metadata_convertor.cc Outdated
Comment thread cpp/CMakeLists.txt
Comment thread cpp/src/phonenumbers/phonenumberutil.cc Outdated
Comment thread cpp/src/phonenumbers/phonenumberutil.cc Outdated
Comment thread cpp/src/phonenumbers/metadata_convertor_short.cc Outdated
Comment thread cpp/src/phonenumbers/shortnumberinfo.h Outdated
@dponomarenko-firstorion

dponomarenko-firstorion commented Dec 1, 2023

Copy link
Copy Markdown
Author

Hello @katbohm @kkeshava and @tvislavski!
Thank you so much for responding to our Pull Request and for comments which helped us to improve this code change!
As requested, I removed two binary *.dat files from repo and made them generated by build rules.
Also I responded to all other comments,
please review our latest changes,
Thanks!

@drudquist

Copy link
Copy Markdown

Hello @katbohm @kkeshava and @tvislavski,

Is there anything else needed for this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants