Refactor `zstash` for improved maintainability

[forsyth2]

Request criteria

• I searched the zstash GitHub Discussions to find a similar question and didn't find it.
• I searched the zstash documentation.
• This issue does not match the other templates (i.e., it is not a bug report, documentation request, feature request, or a question.)

Issue description

@TonyB9000 and I made substantial improvements to zstash's structure in #416 in an effort to better support transfer tracking, but we've also noted things beyond the scope of that PR that could further improve zstash's maintainability. I'm starting this issue to track ideas for such a future refactor (whose purpose would be solely better maintainability, not just in service of fixing a bug).

[forsyth2]

Ideas from @TonyB9000:

Idea 1

Presently, we have functions hpss_get() and hpss_put(). They serve only to feed the word “get” or “put” to hpss_transfer(). They extend the call-stack needlessly. BUT: Rather than eliminate them, I want to make them REAL, and destroy “hpss_transfer()”. This function must juggle “put”, “get”, “either” throughout its body, both extending its size and complicating its analysis. We should:

• Place everything “get” into hpss_get()
• Place everything “put” into hpss_put()
• Place everything non-trivial common to BOTH “get” and “put” into one or more hpss_util functions that get called in hpss_get() and hpss_put(). If trivial, we simply repeat it.

Then, hpss_get() and hpss_put() will actually contain the codes supporting hpss get, and hpss put. I believe this will facilitate code analysis and maintenance. For starters, it will eliminate codes like this:

        transfer_word: str
        transfer_command: str
        if transfer_type == "put":
            transfer_word = "to"
            transfer_command = "put"
        elif transfer_type == "get":
            transfer_word = "from"
            transfer_command = "get"
        else:
            raise ValueError("Invalid transfer_type={}".format(transfer_type))
        logger.info("Transferring file {} HPSS: {}".format(transfer_word, file_path))

Idea 2

If these “transfer_batch” objects exist only for file deletion management, they should be called “deletion_batch” objects. As it stands, one would naturally think that a “transfer_batch” is used by transfer-processing to know what to transfer and when, AND its status. If, in a future refactor, they serve a large role in configuring actual transfer activity, THEN we can name them “transfer_batch” objects.

(This one might make sense to include in #416, but in an effort to get that PR merged by the RC deadline and leave open the option to expand its functionality later, let's leave it as is for now).

[TonyB9000]

@forsyth2 "whose purpose would be solely better readability, not just in service of fixing a bug",

More than just readability, maintainability should also improve when complexity can be reduced.

[forsyth2]

@TonyB9000 Good point, I changed the wording to reflect this. It better captures the importance.

[forsyth2]

This function must juggle “put”, “get”, “either” throughout its body, both extending its size and complicating its analysis.

I originally implemented it this way so as to not repeat code ("Don't Repeat Yourself" DRY principle). But it is a valid criticism that it produces some awkward blocks like:

        if transfer_type == "put":
            transfer_word = "to"
            transfer_command = "put"

But I suppose my counterpoint is we're always going to need to separate based on type of transfer. We have 3 dimensions/axes that come up in if/else blocks here:

• put or get?
• No hpss, hpss, or globus?
• Keep or not keep?

So, that's 2x3x2 combinations = 12 potential functions that could be written out of this.

[forsyth2]

I did a preliminary refactor of hpss_transfer() -- see here, but I don't think it's particularly straightforward to completely remove that function in place of only using hpss_put() and hpss_get().

(Again, this is just an early start; we can do more post-Unified release).

[TonyB9000]

True, we would not want 12 separate functions - but "get vs put" seems major. If we keep it as is, we should simply eliminate "hpss_get()" and "hpss_put()", and instead call "hpss_transfer()" directly..

Thing is, when I see a large "if" block with "if keep and put", and down in that block there are some "return" calls, I am unsure whether we are still in a "get" or "put" state beyond the block. A comment to the effect, "we are still doing get or put" would help clarify. But separate functions makes that unnecessary as well.

I would try to support "Don't Repeat Yourself" by extracting the repeated part into a separate callable function - unless it is a very trivial call.

[TonyB9000]

All of this is post-Unified commentary, just for capture. My plan would be:

1. make two copies of hpss_transfer, one for "get" and one for "put"
2. in "get", remove any code that is purely for "put"
3. in "put" remove any code that is purely for "get"
4. whatever remains in each that is common, seek to extract to separate function(s).

Paraphrasing some famous programmer, "Most problems can be solved with another layer of indirection", but I would add, the layer should do something more significant then merely introduce new function names.