Feature #2751
closedPython SDK behaves appropriately when API server advertises a Keep proxy instead of individual Keep storage servers
Description
- Don't panic just because only one Keep server (proxy) is available.
- Pay attention to
X-Data-Replication
header in server response. - Ensure https works.
Related issues
Updated by Brett Smith over 10 years ago
Reviewing c0c0d76. The code is very clean, so thank you for an easy review. I only have a couple of small points.
shuffled_service_roots has an except:
clause. Can you catch a specific exception here? except:
catches things like KeyboardInterrupt and SystemExit, which is not what you intend here. If you can't name the exception, at least say except Exception:
to avoid that issue.
In KeepTestCase.setUpClass
, would it help improve test isolation to clear the arvados module variables from setUp
instead? Since you're re-doing that in specific test methods, it looks like it could simplify things a bit too. Relatedly, is it worth refactoring all the setup and teardown methods in test_keep_client to a common superclass, so you don't have to repeat it between the test cases?
Updated by Peter Amstutz over 10 years ago
- Changed "except:" to "except Exception:". Thanks for that.
- Refactored test_keep_client a little bit to make it clearer what is going on (and ideally a little more robust, as I realized the tests were order sensitive). It's hard to refactor very much, since tests inherently contain a lot of code that sets up an environment that's very specific to the test.
Updated by Brett Smith over 10 years ago
Peter Amstutz wrote:
- Refactored test_keep_client a little bit to make it clearer what is going on (and ideally a little more robust, as I realized the tests were order sensitive). It's hard to refactor very much, since tests inherently contain a lot of code that sets up an environment that's very specific to the test.
That is helpful, thanks. This looks good to merge to me.
Updated by Anonymous over 10 years ago
- Status changed from New to Resolved
- % Done changed from 67 to 100
Applied in changeset arvados|commit:fd7ac9bf21002cc8a3cdb9a5e16c588ff734dfab.