Skip to content

Add Datetime to Instant converter #376

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 4 commits into from
Apr 6, 2023
Merged

Add Datetime to Instant converter #376

merged 4 commits into from
Apr 6, 2023

Conversation

ArtDu
Copy link
Contributor

@ArtDu ArtDu commented Mar 29, 2023

This converter can support parsing Datetime tarantool type to java Instant. Because Instant doesn't support timezone and offset we can't support full DateTime type features.

Also to test new features we need to have variants of tarantool in CI so we did:

  • Changing tarantool container init to use version depending on CI TARANTOOL_VERSION variable.
  • Updating cartridge-java-testcontainer so that the cartridge container is based on the same image as tarantool container (see tarantool/testcontainers-java-tarantool@12927b9).

But we have one problem with versions. Tarantool on Docker hub has fixed patches only for 2.10 version if we speak about centos versions. Besides 2.10 versions we only can use 1.x, 2.x tags. That means the latest versions, for e.g. 1.10.15, 2.10.6. Then we have two ways:

  1. centos
  • use fixed 2.10
  • 1.x
  1. alpine
  • use fixed 2.8
  • use fixed 2.10
  • use fixed 1.10
  • others

I haven't forgotten about:

  • Tests
  • Changelog
  • Documentation
  • Commit messages comply with the guideline
  • Cleanup the code for review. See checklist

Related issues:
Closes #214

@ArtDu ArtDu changed the title Add datetime converter WIP: Add datetime converter Mar 29, 2023
Copy link
Collaborator

@akudiyar akudiyar left a comment

Choose a reason for hiding this comment

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

LGTM, but the hard-coded version a bit scares me

@@ -9,7 +9,7 @@ abstract class SharedTarantoolContainer {

private static final Logger logger = LoggerFactory.getLogger(SharedTarantoolContainer.class);

protected static final TarantoolContainer container = new TarantoolContainer()
protected static final TarantoolContainer container = new TarantoolContainer("tarantool/tarantool:2.10.5-centos7")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to hardcode the Tarantool version like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't finished it. Just draft 🫠

@ArtDu ArtDu changed the title WIP: Add datetime converter WIP: Add datetime to Instant converter Mar 31, 2023
@ArtDu ArtDu force-pushed the datetimetype branch 2 times, most recently from e5a3030 to 9f4a7ab Compare March 31, 2023 15:52
@ArtDu ArtDu changed the title WIP: Add datetime to Instant converter WIP: Add Datetime to Instant converter Apr 2, 2023
@ArtDu ArtDu changed the title WIP: Add Datetime to Instant converter Add Datetime to Instant converter Apr 2, 2023
@ArtDu ArtDu requested a review from akudiyar April 2, 2023 11:25
@ArtDu ArtDu marked this pull request as ready for review April 2, 2023 11:26
@ArtDu ArtDu mentioned this pull request Apr 2, 2023
4 tasks
@akudiyar akudiyar added this pull request to the merge queue Apr 6, 2023
Merged via the queue into master with commit 61fd3e6 Apr 6, 2023
@akudiyar akudiyar deleted the datetimetype branch April 6, 2023 11:18
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.

Support DATETIME type for tarantool >= 2.10
3 participants