[#746] establish default order for replicas listed by an iRODSDataObject#815
[#746] establish default order for replicas listed by an iRODSDataObject#815d-w-moore wants to merge 7 commits intoirods:mainfrom
iRODSDataObject#815Conversation
Keep in mind that for a minor release, we cannot change the behavior of any public APIs. If the default sorter results in the output being different, then that's a no go. The default sorter must mirror the original behavior.
What are you referring to in regard to deprecation?
What does this mean? |
We'd discussed in the old issue/PR convo's whether we might not just deprecate the
Just that .replicas[0].FIELD is mirrored in .FIELD, but that is pretty natural. |
|
@korydraughn - I'm fine with changing the default order back to sorting on replica number for this minor release, even if it will allow attributes such as |
| # Ensure that one of the replicas is stale, to test proper sorting. | ||
| with data.open('a', **{kw.RESC_NAME_KW: newResc1}) as f: | ||
| f.write(b'.') | ||
| time.sleep(2) | ||
|
|
||
| # Voting should ensure exactly two good replicas of the three. | ||
| data.replicate(resource=newResc2) |
There was a problem hiding this comment.
Please add an assertion which proves a replica is stale.
| self.manager = manager | ||
| if parent and results: | ||
| self.collection = parent | ||
| results = sorted(results, key=(replica_sort_function or _DEFAULT_SORT_KEY_FN)) |
There was a problem hiding this comment.
I think the only way to support this in a minor release is to provide an opt-in which changes the default behavior.
| test_put__issue_722(self) | ||
|
|
||
| def test_default_sorting_of_replicas__issue_647(self): | ||
| @unittest.skipIf(irods.version.version_as_tuple() < (4,), 'too soon for this test.') |
There was a problem hiding this comment.
Consider changing the message to something like the following.
Relies on backward incompatible changes. Disabled until PRC 4
|
|
||
| def test_default_sorting_of_replicas__issue_647(self): | ||
| @unittest.skipIf(irods.version.version_as_tuple() < (4,), 'too soon for this test.') | ||
| def test_modified_default_sorting_of_replicas__issue_647(self): |
There was a problem hiding this comment.
Looks like we need another test for the sorter option?
Alternatively, you can change the behavior of the test such that it covers PRC 3 and PRC 4. For example:
if irods.version.version_as_tuple() < (4,):
data = self.sess.data_objects.get(data.path, sorter=<fn>)
else:
data = self.sess.data_objects.get(data.path)Doing that implies the name of the test would need to change as well.
Oh right. That still sounds like an acceptable approach.
I'm not yet convinced that is the proper approach. Feels like it should be handled via support functions which simplify the find-replica step. Do instances of |
2 participants
The parent data object's
modify_timeandreplica_statusfields , as well as some others, actually pertain more to individual replicas.#747 was an old PR meant to address the issue and contains much discussion as well.
On consideration, I think a minor release is the proper place to address this, and I'm doing it by
data_objects.get( or anytime running theiRODSDataObjectconstructor) sorts replicas of the data object first by the replica-"goodness" and secondly by reverse chronology of the replicamodify_time(ie most recent first.) The replica at array position [0] will then determine the values of the fields discussed above.modify_timeandreplica_statusto be accessed from the "head" object.So, this PR replaces the old one, #747 , due to being new work and being based on top of source code conveniently ruff-formatted.