-
Notifications
You must be signed in to change notification settings - Fork 998
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
PHOENIX-6963 : Bootstrap last ddl timestamp for indexes #1612
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the PR looks good. Few minor comments. Thank you @palashc !
phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
private static void bootstrapLastDDLTimestamp(Connection metaConnection, String[] tableTypes) throws SQLException { | ||
String tableTypesString = Stream.of(tableTypes).collect(Collectors.joining("','", "'", "'")).toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
learned something new on how to use prefix and suffix along with delimiter. Thank you.
phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java
Outdated
Show resolved
Hide resolved
@@ -4122,6 +4122,7 @@ protected PhoenixConnection upgradeSystemCatalogIfRequired(PhoenixConnection met | |||
metaConnection = addColumnsIfNotExists(metaConnection, PhoenixDatabaseMetaData.SYSTEM_CATALOG, | |||
MIN_SYSTEM_TABLE_TIMESTAMP_5_2_0, | |||
PhoenixDatabaseMetaData.STREAMING_TOPIC_NAME + " " + PVarchar.INSTANCE.getSqlTypeName()); | |||
UpgradeUtil.bootstrapLastDDLTimestampForIndexes(metaConnection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how this UpgradeUtil code works and when it is executed? If it is executed after the upgrade then the server version would be equal to MIN_SYSTEM_TABLE_TIMESTAMP_5_2_0. But adding DDL timestamps to table/view happened in 4.16.0 version and there also we have similar condition currentServerSideTableTimeStamp < MIN_SYSTEM_TABLE_TIMESTAMP_4_16_0
so this code clearly works.
assertEquals(tableTS, actualTableTS); | ||
assertEquals(viewTS, actualViewTS); | ||
assertEquals(0L, actualIndexTS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should actualIndexTS be NULL or 0? I think it should be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is returned using a rs.getLong()
in getLastTimestampForMetadata
call which returns 0 if the actual value was sql null.
// bootstrap last ddl timestamp for indexes | ||
UpgradeUtil.bootstrapLastDDLTimestampForIndexes(conn.unwrap(PhoenixConnection.class)); | ||
actualIndexTS = getLastTimestampForMetadata(conn, schemaName, indexName, PTableType.INDEX); | ||
assertEquals(indexTS, actualIndexTS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I am missing something.
indexTS
is the timestamp after we set LAST_DDL_TIMESTAMP to null via nullDDLTimestamps
method. So it should be NULL.
actualIndexTS
is the timestamp after we call UpgradeUtil.bootstrapLastDDLTimestampForIndexes
. So it should the LAST_ROW_TIMESTAMP() for this index.
So how are they supposed to be equal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shahrs87 indexTS
is the PHOENIX_ROW_TIMESTAMP() of the header row of the index in syscat (it is the timestamp of the empty column) and indicates the last time the row was modified. After we call UpgradeUtil.bootstrapLastDDLTimestampForIndexes
, this same value for the row is used to populate the LAST_DDL_TIMESTAMP
column which is captured in actualIndexTS
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my bad, I thought indexTS is calculated from getLastTimestampForMetadata
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the PR looks good. Few minor comments. Thank you @palashc !
phoenix-core/src/it/java/org/apache/phoenix/end2end/UpgradeIT.java
Outdated
Show resolved
Hide resolved
@palashc In the last jenkins run, there were around 16 checkstyle errors. If those are related to your patch then please fix it otherwise +1 ltgm. |
I don't have commit rights, @tkhurana can you please review this patch too? Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 pending fixing checkstyle warnings. Thank you @palashc
No description provided.