Skip to content

Convert All Usage of LocalDateTime to Instant #456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 31 commits into from
Jun 16, 2025

Conversation

emmyzhou-db
Copy link
Contributor

@emmyzhou-db emmyzhou-db commented Jun 4, 2025

What changes are proposed in this pull request?

This pull request refactors the Databricks Java SDK to replace all usages of LocalDateTime with Instant for representing date-time values. The primary motivation is to ensure that all date-time handling is unambiguous, always in UTC, and consistent across the SDK.

How does time currently operate in the SDK?

Currently, tokens in the SDK use LocalDateTime for expiry timestamps, which by itself is timezone-agnostic and could be ambiguous. Thus, it is not clear how time is handled from the type alone and we need to look at the implementation details of the ClockSupplier used.

The SDK currently uses system time through the SystemClockSupplier class as defined here. This implementation of the ClockSupplier interface returns Clock.systemDefaultZone(), meaning it uses the actual system time from the operating system. The SystemClockSupplier is the used throughout the SDK to conduct OAuth token expiration checks as seen here in the isExpired() method of the Token class. While LocalDateTime doesn't explicitly store timezone information, the system's timezone is being implicitly used for these comparisons through Clock.systemDefaultZone(). In other words, token expiry times are implicitly measured in the system's timezone.

Converting to UTC

To convert these expiry times to UTC while maintaining the SDK's current behavior, we need to handle two distinct scenarios:

1. Tokens with expires_in

  • The response includes the time until expiry.
  • Conversion:
    Add the expires_in value to the current UTC time.
  • Result:
    This approach maintains the original expiration behavior while using UTC internally.

2. Tokens with expires_on (e.g., Azure CLI, Databricks CLI)

With Time Zone Indicator

  • Tokens (such as those from the Databricks CLI) provide expiry times in RFC 3339 format, including a time zone indicator.
  • Conversion:
    These can be easily converted to UTC using standard library methods.

Without Time Zone Indicator

  • Example: The expiresOn field from Azure CLI
    Reference
  • Tokens provide expiry timestamps without explicit time zone information.
  • The timestamp is implicitly in the system’s local time zone.
  • Conversion steps:
    1. Interpret the timestamp as local time.
    2. Add the system timezone.
    3. Convert this to UTC.
      We do this with LocalDateTime.atZone(systemDefault).toInstant()

How is this tested?

Future Work

  • The SDK currently parses expiry strings from external sources like Azure CLI
  • Azure CLI provides two timestamp fields:
    • expiresOn: Local datetime without timezone (maintained for backward compatibility)
    • expires_on: POSIX timestamp (seconds since epoch) - preferred format

Current Implementation

  • The Java SDK currently uses the deprecated expiresOn field
  • LocalDateTime creates timezone interpretation challenges

To Do

  1. Phase out expiresOn
    This legacy field should be deprecated in favor of expires_on
  2. Update parsing logic
    Future PRs should prioritize using expires_on (POSIX timestamp) which:
    • Provides unambiguous UTC-based expiration time
    • Avoids timezone conversion complexities

NO_CHANGELOG=true

@emmyzhou-db emmyzhou-db requested a review from parthban-db June 4, 2025 09:22
Copy link
Contributor

@renaudhartert-db renaudhartert-db left a comment

Choose a reason for hiding this comment

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

LGTM modulo resolution of the fallback discussion.

@emmyzhou-db emmyzhou-db requested a review from parthban-db June 4, 2025 16:59
@parthban-db parthban-db temporarily deployed to test-trigger-is June 5, 2025 11:51 — with GitHub Actions Inactive
@parthban-db parthban-db temporarily deployed to test-trigger-is June 5, 2025 11:52 — with GitHub Actions Inactive
Copy link

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/sdk-java

Inputs:

  • PR number: 456
  • Commit SHA: f58a2f6fde4881e0a9f651287d4a4261e7345343

Checks will be approved automatically on success.

@emmyzhou-db emmyzhou-db added this pull request to the merge queue Jun 16, 2025
Merged via the queue into main with commit e51c2c6 Jun 16, 2025
15 checks passed
@emmyzhou-db emmyzhou-db deleted the emmyzhou-db/localdatetime-to-instant branch June 16, 2025 09:19
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