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

Server config #1249

Merged
merged 2 commits into from
Jan 18, 2018
Merged

Server config #1249

merged 2 commits into from
Jan 18, 2018

Conversation

csordasmarton
Copy link
Contributor

Closes #1187

Copy link
Contributor

@gyorb gyorb left a comment

Choose a reason for hiding this comment

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

We should check if in the workspace there is a deprecated session_config. In the current case if the newer server is started a server_config will be created and used. The configuration options in the previous session_config will be lost and not used. At least we should warn the user to check the new configuration and set the config values if needed.

@@ -1898,6 +1900,24 @@ def massStoreRun(self, name, tag, version, b64zip, force):
raise shared.ttypes.RequestFailed(
shared.ttypes.ErrorCode.GENERAL,
'Storage of ' + name + ' is already going!')

max_run_count = self.__manager.get_max_run_count()
# If max_run_count is not set in the config file, it will allows
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a typo allow only.

The `max_run_count` section of the config file controls the maximum storable
runs.

If this field is not presented in the config file or the value of this
Copy link
Contributor

Choose a reason for hiding this comment

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

not present

* [Authentication](#authentication)

## Run limitation
The `max_run_count` section of the config file controls the maximum storable
Copy link
Contributor

Choose a reason for hiding this comment

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

"Maximum storable" can be misinterpreted with the meaning of "concurrent storage operations running at once".

"""
Tries to load the session config file which should be a
valid json file, if loading fails returns an empty dict.
"""

scfg_dict = {}
try:
with open(session_cfg_file, 'r') as scfg:
with open(server_cfg_file, 'r') as scfg:
Copy link
Contributor

Choose a reason for hiding this comment

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

We are having more and more JSON files to store configurations. I wonder if we do have a utility that loads such file and handle the errors somewhere or in case we don't it might be worth to have one and reduce code duplication.

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't "more and more" file, the file just got renamed and added some extra configuration variables. My staled patch, #1172, contains a utility function that allows you to load a JSON file and get a default dict if the loading failed.

Copy link
Contributor

@whisperity whisperity left a comment

Choose a reason for hiding this comment

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

Two minor comments, the rest looks good.

@@ -68,7 +68,7 @@ def wrapper(self, *args, **kwargs):
print('API call error: ' + funcName)
print(str(reqfailure))

raise
exit(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change neccessary? Is it really a good idea to not let the client code who issued the Thrift req do any sort of error management or prompt the user to give a proper request (e.g. if run name is bogus or something)?

"""

session_config_filename = "session_config.json"
session_config_filename = "server_config.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

Variable name remained the same, leading to possible confusion.

@csordasmarton csordasmarton force-pushed the server_config branch 2 times, most recently from a54adef to ced3920 Compare January 18, 2018 08:42
 * Rename session_config.json to server_config.json.

with open(session_cfg_file, 'r+') as scfg:
with open(server_cfg_file, 'r+') as scfg:
__scfg_original = scfg.read()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use load_json_or_empty here? Do we need the seek and __scfg_original?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, do we use scfg for anything at all below?

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't seem like it. This is a copy from the real session manager file, in which case it was used.

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

Successfully merging this pull request may close these issues.

4 participants