Skip to content

Conversation

@christolis
Copy link
Member

From a recent conversation:

[13:09]Zabuzard: while at it, ZonedDateTime should be replaced by Instant in that file
[13:10]Zabuzard: does someone want to tackle that quickly or else we need to make a github issue 🙂
[13:56]christolis: what makes this change worthwhile?
[14:01]Zabuzard: its just the wrong class for the job
[14:01]Zabuzard: there is no reason to use ZDT in this case
[14:01]Zabuzard: Instant is the correct class
[14:02]Zabuzard: shouldnt require any deeper changes in this case, just a quick replacement should work```

Signed-off-by: Chris Sdogkos <work@chris-sdogkos.com>
@christolis christolis requested a review from a team as a code owner December 29, 2025 12:13
Copy link

@tj-wazei tj-wazei left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

ZonedDateTime savedDate =
getZonedDateTime(rssRecord.getLastDate(), dateFormatterPattern);
return Optional.of(savedDate);
return Optional.of(Instant.parse(rssRecord.getLastDate()));
Copy link
Member

Choose a reason for hiding this comment

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

this will throw java.time.format.DateTimeParseException, we should handle it instead of just silenty eating the exception. Something like

catch (DateTimeParseException ignored) {
        try {
            //TODO: Should be removed once we cycle through all stored ZDT string instances
            Instant parsed_instant = ZonedDateTime.parse(raw).toInstant();
            //INFO log here converted from raw -> instant
            // save parsed_instant to database so this doesnt trigger exception on next run
            return Optional.of(parsed_instant); //
        } catch (DateTimeParseException e) {
            return Optional.empty();
        }
    }
    

Copy link
Member

@ankitsmt211 ankitsmt211 left a comment

Choose a reason for hiding this comment

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

everything looks good but we should handle the outdated timestamps stored in DB

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.

3 participants