Skip to content
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

Implement Materialized View GRACE PERIOD #15842

Merged
merged 9 commits into from
Mar 11, 2023

Conversation

findepi
Copy link
Member

@findepi findepi commented Jan 25, 2023

Description

Building on top of syntax added in #15104, fulfilling the important part of #15326, this PR delivers GRACE PERIOD support in the engine, exposing it to connectors and supporting it in Iceberg materialized views.

Additional context and related issues

Fixes #15326
Fixes #11698

Release notes

# General
* Support `GRACE PERIOD` clause in `CREATE MATERIALIZED VIEW` task.
  For backward compatibility, the existing materialized views are interpreted as
  having GRACE PERIOD of zero duration, however new materialized views have
  unlimited grace period by default. This is a backwards incompatible change and
  the previous behavior can temporarily be restored using
  `legacy.materialized-view-grace-period` configuration property or using
  `legacy_materialized_view_grace_period` session property.

# Iceberg
* Support materialized views with grace period defined.

@findepi
Copy link
Member Author

findepi commented Jan 25, 2023

Some preparatory commits extracted to #15843, #15844

@findepi
Copy link
Member Author

findepi commented Jan 27, 2023

CI #12535

@findepi

This comment was marked as outdated.

@findepi
Copy link
Member Author

findepi commented Mar 9, 2023

just rebased, still ready for review

@github-actions github-actions bot added the iceberg Iceberg connector label Mar 9, 2023
core/trino-spi/pom.xml Outdated Show resolved Hide resolved
}

// Can be negative
// TODO should we compare lastFreshTime with session.start() or with current time? The freshness is calculated with respect to current state of things.
Copy link
Member

Choose a reason for hiding this comment

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

Most predictable would be to cache freshness within a session and compare agains session start time.
But IDK if there are case when we compute the value within single sessions more than once. If not then it probably does not matter much.

Copy link
Member Author

Choose a reason for hiding this comment

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

i will leave this as is

@findepi
Copy link
Member Author

findepi commented Mar 10, 2023

thanks @losipiuk for you review.
Changes squashed directly since they are what you wanted them to be.
The only new code is Add Preconditions in io.trino.spi.connector package.
THere is one fixup which i mis-titled, so will force push once more.

Expose the GRACE PERIOD in `ConnectorMaterializedViewDefinition` so that
connectors can store the information.
Let connector inform the engine how fresh the materialized view is. This
is required to implement materialized view handling as per the new GRACE
PERIOD syntax definition.

Note: the property is called "last fresh time" and not "last refresh
time". This is in support for connectors which know when view became no
longer fresh. For example, a materialized view may have been refreshed a
day ago, but only an hour ago one of its base tables changed. Then the
materialized view is stale and "last fresh time" is one hour ago, not
one day ago.
@findepi
Copy link
Member Author

findepi commented Mar 11, 2023

Error:  io.trino.plugin.sqlserver.TestSqlServerWithSnapshotIsolation.init  Time elapsed: 0.423 s  <<< FAILURE!
...
Caused by: org.rnorth.ducttape.RetryCountExceededException: Retry limit hit with exception
	at org.rnorth.ducttape.unreliables.Unreliables.retryUntilSuccess(Unreliables.java:88)
	at org.testcontainers.containers.GenericContainer.doStart(GenericContainer.java:334)
	... 28 more
Caused by: org.testcontainers.containers.ContainerLaunchException: Could not create/start container
	at org.testcontainers.containers.GenericContainer.tryStart(GenericContainer.java:542)
	at org.testcontainers.containers.GenericContainer.lambda$doStart$0(GenericContainer.java:344)
	at org.rnorth.ducttape.unreliables.Unreliables.retryUntilSuccess(Unreliables.java:81)
	... 29 more
Caused by: java.lang.IllegalStateException: Container exited with code 1
	at org.testcontainers.containers.GenericContainer.tryStart(GenericContainer.java:514)
	... 31 more

unrelated, ignored

@findepi findepi merged commit 62456c7 into trinodb:master Mar 11, 2023
@findepi findepi deleted the findepi/grace-iceberg branch March 11, 2023 21:29
@github-actions github-actions bot added this to the 411 milestone Mar 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 participants