[#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 10 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 |
|
|
||
| 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.
There was a problem hiding this comment.
We're now testing both ways, with and without the replica_sort_function.
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 |
d-w-moore
force-pushed
the
746.m
branch
2 times, most recently
from
0cc7227 to
a9c4e99
Compare
| # 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) |
There was a problem hiding this comment.
Let's put a comment explaining that this sleep is to ensure the replicas on newResc1 and newResc2 have different modify times
|
|
||
|
|
||
| _REPL_STATUSES = (1, 0, 2, 3, 4) | ||
| _REFERENCE_DATETIME = datetime.datetime(1970, 1, 1, 0, 0, 0, tzinfo=datetime.timezone.utc) |
There was a problem hiding this comment.
Could we use this instead to get this value?
datetime.fromtimestamp(0, timezone.utc)https://docs.python.org/3/library/datetime.html#datetime.datetime.fromtimestamp
| return "<{}.{} {}>".format(self.__class__.__module__, self.__class__.__name__, self.resource_name) | ||
|
|
||
|
|
||
| _REPL_STATUSES = (1, 0, 2, 3, 4) |
There was a problem hiding this comment.
As a possible future improvement, it may be beneficial to represent replica statuses as a proper enumeration with names and all that.
3 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.