Skip to content

cadc-registry: improve client error handling and retry behaviour#236

Merged
pdowler merged 4 commits into
opencadc:mainfrom
pdowler:main
Jun 22, 2026
Merged

cadc-registry: improve client error handling and retry behaviour#236
pdowler merged 4 commits into
opencadc:mainfrom
pdowler:main

Conversation

@pdowler

@pdowler pdowler commented Jun 18, 2026

Copy link
Copy Markdown
Member

No description provided.

@Zarquan Zarquan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. Just a couple of comments to clarify edge case behaviour.
(*) code review only - I haven't built and run the tests myself.
Let me know if you want me to do that.

Hand crafted by a human, no AI used in this review.
AIMetrics: []

Comment thread cadc-registry/src/main/java/ca/nrc/cadc/reg/client/RegistryClient.java Outdated
Comment thread cadc-registry/src/main/java/ca/nrc/cadc/reg/Capability.java

@Zarquan Zarquan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks fine. Just me being pedantic about hard coded values that aren't covered in the comments.

Hand crafted by a human, no AI used in this review.
AIMetrics: []


/**
* HTTP read timeout in milliseconds (default: 60000).
* HTTP read timeout in milliseconds (default: 30000).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

readTimeout defaults to 6,000 not 30,000
This method sets readTimeout to the specified value, but it also sets readCapabilitiesTimeout to 5 * readTimeout, which does indeed default to 30,000.

The 5 * readTimeout bit is hard coded twice, once in the initialisation and once in this method, but it isn't mentioned in the comments.


Trying to find out where the 30,000 value in this debug statement came from.

    2026-06-18 19:01:19.889[Test worker] DEBUG CachingFile - loadRemoteContent: https://ws.cadc-ccda.hia-iha.nrc-cnrc.gc.ca/argus/capabilities 3000/30000

https://github.com/opencadc/reg/actions/runs/27782591339/job/82210973963?pr=236#step:4:1117

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is subtle to get a good/robust behaviour. I'm improving the javadocs to note these different timeouts.

That 3000/30000 will be a read of a capabilities document, which has the same 3000 connection timeout and 5*6000 = 30000 read timeout (I made read timeout longer than connection timeout because the main improvement I am after is to shorten the delays when a reg service or fronted proxy is offline). So that looks right to me.

@Zarquan

Zarquan commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Something that I missed when I was working on this.

CachingFile.loadRemoteContent() declares that it throws IOException, RemoteServiceException, and TransientException

private void loadRemoteContent(OutputStream dest) throws IOException, RemoteServiceException, TransientException

CachingFile.getContent() declares that it throws IOException

public String getContent() throws IOException

but it calls loadRemoteContent() inside a try/catch that only catches IOException

    try {
        loadRemoteContent(fos);
    } catch (IOException e) {

potentially leaving RemoteServiceException, and TransientException uncaught.

Which implies that RemoteServiceException and TransientException are runtime exceptions that don't need to be declared in the public facing getContent(), but they do need to be declared in loadRemoteContent() so they can be throw using a class cast

    if (download.getThrowable() instanceof TransientException) {
        throw (TransientException) download.getThrowable();
    }
    if (download.getThrowable() instanceof RemoteServiceException) {
        throw (RemoteServiceException) download.getThrowable();
    }

Is it is safe to assume that RemoteServiceException and TransientException are covered by tests in the ca.nrc.cadc.net.HttpGet package, and we don't need to add explicit tests that trigger them in CachingFile.

Hand crafted by a human, no AI used in this review.
AIMetrics: []

@pdowler

pdowler commented Jun 22, 2026

Copy link
Copy Markdown
Member Author

thnx for thorough look at this - error handling (and retry) is always a tricky bit to make generally right)

I have declared and documented the core exceptions thrown by CachingFile.getContent(). Some are RuntimeExceptions but I sometimes declare them for clarity when the caller may want to differentiate between different failure modes but not know what to catch.

I really wanted to capture some call history so that RegistryClient lookups wouldn't get stuck making even short blocking calls to a service that was down, but that bit is hard given how RegistryClient is used in code (instantiated in request thread, called, thrown away)... so for this PR I'm just trying to tighten up the behaviour.

In that failure mode it is 2 attempts (max retries 1) so one of:

3 sec connection timeout + 1 sec wait + 3 sec connection timeout

or if it is just the backend container down:

front end proxy timeout + 1 sec wait + front end proxy timeout

(which for our haproxy instances is roughly 7 + 1 + 7 = 15 sec, still a lot of blocking)


In a future PR I will try something like

  • maybe touch the cache to unexpire it for a little while?
  • track and sort by responsiveness of reg services (shared state in the JVM)??

@pdowler pdowler merged commit a279d80 into opencadc:main Jun 22, 2026
1 check passed
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