Add reconfig poll patch #1801
Add reconfig poll patch #1801
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
| @@ -90,6 +90,9 @@ class FileSystemStoragePathSource : public Source<StoragePath> { | |||
| // such child. | |||
| Status PollFileSystemAndInvokeCallback(); | |||
|
|
|||
| // Logs servable version information | |||
| void LogVersions(const std::vector<ServableData<StoragePath>>& versions); | |||
There was a problem hiding this comment.
I don't see any reasons to make it a class method as its logic doesn't involve reading/manipulating class state. Can we remove this declaration from the .h file and add it as a function wrapped with anonymous namespace in the .cc file (the only place where it's used)?
| CallAspiredVersionsCallback(servable, versions); | ||
| } | ||
| return Status::OK(); | ||
| } | ||
|
|
||
| void FileSystemStoragePathSource::LogVersions( |
There was a problem hiding this comment.
As per the other comment, let's put this implementation outside of the class definition, under anonymous namespace.
| CallAspiredVersionsCallback(servable, versions); | ||
| } | ||
| return Status::OK(); | ||
| } | ||
|
|
||
| void FileSystemStoragePathSource::LogVersions( | ||
| const std::vector<ServableData<StoragePath>>& versions) { | ||
| for (const ServableData<StoragePath> &version : versions) { |
| versions_by_servable_name; | ||
| Status FailIfZeroVersions(const FileSystemStoragePathSourceConfig& config, | ||
| std::map<string, std::vector<ServableData<StoragePath>>>& | ||
| versions_by_servable_name) { | ||
| TF_RETURN_IF_ERROR( | ||
| PollFileSystemForConfig(config, &versions_by_servable_name)); |
There was a problem hiding this comment.
It seems like PollFileSystemForConfig is already called in line 344. So it should be fine to remove it from here and pass a read-only arg versions_by_servable_name to FailIfZeroVersions.
This patch is essentially porting the patch submitted by @njhill in PR #1518 to
2.4.0versionCurrently when the configured model list is updated via a call to handleReloadConfigRequest, the request thread blocks until any newly added models become available.
Their availability however depends on the filesystem polling thread rescanning the filesystem at some periodic interval, meaning that there's an arbitrary delay before the requested changes actually take effect and the RPC returns.
This problem may not be very noticeable with the default polling interval of 1 second, but seems undesirable for longer intervals and in particular makes API-based dynamic reconfiguration incompatible with the --file_system_poll_wait_seconds=0 setting (in this case all handleReloadConfigRequest calls time-out and do not take effect).
Fixes #1519