Explicitly notify user if compiled bot too large #288
Conversation
apiserver/worker/worker.py
Outdated
|
|
||
| For our reference, here is the trace of the error: | ||
| """ | ||
|
|
There was a problem hiding this comment.
Given that the size is now explicitly checked before upload, I think I'd leave out the part about "likely the final compiled bot was too large".
apiserver/worker/backend.py
Outdated
| @@ -6,6 +6,9 @@ | |||
| from time import gmtime, strftime | |||
|
|
|||
|
|
|||
| MAX_BOT_UPLOAD_SIZE = 100 * 1024 * 1024 | |||
There was a problem hiding this comment.
Might want to leave a note here and in apiserver/worker/config.py near MAX_COMPILED_BOT_UPLOAD_SIZE that the values should match.
apiserver/worker/backend.py
Outdated
| @@ -95,6 +101,8 @@ def storeBotRemotely(user_id, bot_id, zip_file_path): | |||
| }, | |||
| files={"bot.zip": zip_contents}) | |||
| print("Posting compiled bot archive %s\n" % r.text) | |||
| # We don't check the status code since we instaed check the | |||
| # bot hash. | |||
There was a problem hiding this comment.
There are many error codes that are unlikely to resolve on retry (almost by definition pretty much all the 4xx codes) and we could bail out without trying another 99 times. Also any response error that is likely temporary probably deserves a backoff before retrying.
But leaving it for now is fine, the other changes definitely make it much better than what happens now.
|
Thanks for the feedback. I changed it to back off uploads, and reduced the number of tries to 10 consequently. |
apiserver/apiserver/config.py
Outdated
| @@ -16,6 +16,7 @@ | |||
| # Flask settings | |||
| # Max size of an upload, in bytes | |||
| MAX_BOT_UPLOAD_SIZE = 20 * 1024 * 1024 | |||
| # Needs to match corresponding value in apiserver/config.py | |||
There was a problem hiding this comment.
Why does this need to match the value in itself? IRL recursion :P
apiserver/worker/backend.py
Outdated
|
|
||
|
|
||
| # Needs to match corresponding value in apiserver/config.py | ||
| MAX_BOT_UPLOAD_SIZE = 100 * 1024 * 1024 |
There was a problem hiding this comment.
Rather than define it twice can't we just pick it from there/another central location for both?
There was a problem hiding this comment.
Yeah, the issue is that the worker doesn't assume it has access to the server codebase (though this happens to be true currently). Perhaps we can make it part of the config.json, or have a server endpoint that returns the value?
There was a problem hiding this comment.
Config.json would be fine :)
apiserver/worker/backend.py
Outdated
|
|
||
| # Try again if local and remote hashes differ | ||
| if local_hash != getBotHash(user_id, bot_id): | ||
| print("Hashes do not match! Redoing file upload...\n") | ||
| iterations += 1 | ||
| sleep(backoff) | ||
| if backoff < 32: |
|
Added small comments. I'm up for approving when they're addressed :) |
|
Updated, thanks for the review! |
apiserver/worker/backend.py
Outdated
|
|
||
| # Needs to match corresponding value in apiserver/config.py | ||
| # Default value, 100 MiB | ||
| MAX_BOT_UPLOAD_SIZE = 100 * 1024 * 1024 |
There was a problem hiding this comment.
At first I thought this was now redundant, but I see it's used as a default when the config doesn't define a value. The comment should probably be updated to reflect that.
apiserver/worker/backend.py
Outdated
|
|
||
|
|
||
| with open("config.json") as configfile: | ||
| config = json.load(configfile) | ||
| MANAGER_URL = config["MANAGER_URL"] | ||
| SECRET_FOLDER = config["SECRET_FOLDER"] | ||
| CAPABILITIES = config.get("CAPABILITIES", []) | ||
| MAX_BOT_UPLOAD_SIZE = config.get("MAX_BOT_UPLOAD_SIZE", | ||
| MAX_BOT_UPLOAD_SIZE) |
There was a problem hiding this comment.
This should probably keep the default value if the config value is None.
Fixes #287