Skip to content

Recursive Runner class to get report on dir size - #277

Open
aratikakadiya wants to merge 16 commits into
opencadc:mainfrom
aratikakadiya:directory-size-report
Open

Recursive Runner class to get report on dir size#277
aratikakadiya wants to merge 16 commits into
opencadc:mainfrom
aratikakadiya:directory-size-report

Conversation

@aratikakadiya

Copy link
Copy Markdown
Contributor

No description provided.

@aratikakadiya
aratikakadiya marked this pull request as draft June 9, 2026 21:06
@aratikakadiya
aratikakadiya marked this pull request as ready for review June 19, 2026 22:28

@pdowler pdowler left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just review of main logic so far

- added support to dest(data node) param to the RecursiveNodeSizeRunner to upload the nodesize report.
- Updated int tests for the runner + endpoint.
- Removed sorting for node-size report endpoint.
- Adapted runner to the new HttpUpload functionality - client-managed.
- Refactored the code to use OutputStreamWrapper to upload the report.
* Subclasses may override to attach additional UWS Result entries
* (e.g., computed metrics, reports). Default: no extra results.
*/
protected List<Result> getAdditionalResults() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed and used now output is written to a storage URL?

private Exception scanException = null;

@Override
public void setJob(Job job) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is best to do this at the start of performAction and not in setJob; this method should probably not throw

I realise the other impls do some work in setJob and I will fix that and not override this method.

reason: exception throwing and handling needs to be in JobRunner.run(); the base class does all that and calls performAction() and handles all failures from it

putURL
);
upload.setRequestProperty("Content-Type", CONTENT_TYPE);
upload.run();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I grok how the above expression will work. upload.run() will make the call to putURL, check errors, and then open the output and call OutputStreamWrapper.write(OutputStream). The thing I don't like about the above expression style is it never actually says it is implementing that write method and the stream -> in the HttpUpload ctor call is very subtle. aside: the out variable here is actually a member of the outer class; I usually try to make assignment to class variables stand out with this.out = ....

This version has made the complication of trying to do a lazy init and use OutputStreamWrapper clear. I think there is a way to do it that would work in the typical case (depth=0 and large allocation)...

I think this can be made more clear with a concrete nested class eg:

private class DynamicReportOutput implements OutputStreamWrapper {
    
    private PrintWriter output;

    DynamicReportOutput(ContainerNode cn, Subject caller, int depth) {
        // TODO: scan up to first output here??
    }

    public void write(OutputStream ostream) {
        this.output = new PrintWriter(new OutputStreamWriter(ostream));
        ...
    }
}

I would probably move as much state as possible inside this nested class, and maybe even the accumulate and output methods from the parent (eg the Writer declared above).

I think we could make this do a lazy init with a method in the nested class that scans/accumulates up to the point where it has to do output and then stops. At that point the HttpUpload is created and run(). I think doing that in the ctor would make usage very straightforward... this is typical of a dynamic Iterator (they advance to the first item in the ctor).

- Refactored the code to use concrete class for the OutputStreamWrapper to upload the report. It is designed to initiate node traversal until first report to be written is encountered. and resume traversal when upload is ready with the open connection and output stream.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants