Skip to content

Conversation

@thjarvin
Copy link
Contributor

@thjarvin thjarvin commented May 9, 2025

Fixes AB#58303

@thjarvin thjarvin requested a review from haphut May 9, 2025 12:25
Copy link
Contributor

@haphut haphut left a comment

Choose a reason for hiding this comment

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

Great! Consider squashing at least the bug fix commit. And maybe version to 3.0.0 without RC as it's a breaking change?

LGTM otherwise.

}


public void firstExecuteQueryThenReleaseDbResourcesAndThenHandleResults(final AbstractResultSetProcessor processor) {

Choose a reason for hiding this comment

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

@haphut I'm trying to understand the idea here. this method looks exactly the same as the one above, executeAndProcessQuery, but it calls closeQuery only when an exception occurs, while the other does it always, in the finally block.
Is that intended?
To me it looks like we need to remove this method as the sequence described by the method name:
firstExecuteQuery_ThenReleaseDbResources_AndThenHandleResults
is achieved in JourneyResultSetProcessor#processResultSet method that:

  1. collects all items in memory
  2. closes the query
  3. saves them to redis

Next, would be to apply the same sequence in the other two processors, MetroJourneyResultSetProcessor and StopResultSetProcessor


log.info("All data processed, thank you.");
} catch (SQLServerException sqlServerException) {
String msg = "SQLServerException during query, Driver Error code: "

Choose a reason for hiding this comment

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

This exception should propagate, so it gets caught in the main method, so that the pod would exit with a status code of 1, that would mark it as failed, so the cron job can be retried. We should then configure some retries at the cronjob level.
@haphut does it make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

@chihaiaalex chihaiaalex dismissed their stale review October 10, 2025 05:33

all covered

@chihaiaalex chihaiaalex merged commit dbeb5c6 into aks-dev Oct 10, 2025
8 checks 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.

4 participants