[
https://issues.apache.org/jira/browse/PHOENIX-6186?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17231043#comment-17231043
]
ASF GitHub Bot commented on PHOENIX-6186:
-----------------------------------------
shahrs87 commented on a change in pull request #935:
URL: https://github.com/apache/phoenix/pull/935#discussion_r522489964
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/DropColumnMutator.java
##########
@@ -268,7 +272,19 @@ public MetaDataMutationResult
validateAndAddMetadata(PTable table,
}
}
- tableMetaData.addAll(additionalTableMetaData);
+ if (isDroppingColumns) {
+ //We're changing the application-facing schema by dropping a
column, so update the DDL
+ // timestamp to current _server_ timestamp
+ if (MetaDataUtil.isTableQueryable(table.getType())) {
+ long serverTimestamp =
EnvironmentEdgeManager.currentTimeMillis();
+
additionalTableMetaData.add(MetaDataUtil.getLastDDLTimestampUpdate(tableHeaderRowKey,
+ clientTimeStamp, serverTimestamp));
+ }
+ //we don't need to update the DDL timestamp for any child views we
may have, because
+ // when we look up a PTable for any of those child views, we'll
take the max timestamp
+ // of the view and all its ancestors
+ tableMetaData.addAll(additionalTableMetaData);
Review comment:
`tableMetaData.addAll(additionalTableMetaData);`
The above line won't be executed if isDroppingColumns is false. Earlier it
was getting executed even if we are dropping columns.
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/AddColumnMutator.java
##########
@@ -293,7 +293,9 @@ public MetaDataMutationResult validateAndAddMetadata(PTable
table, byte[][] rowK
List<ImmutableBytesPtr> invalidateList,
List<Region.RowLock>
locks,
long clientTimeStamp,
- long clientVersion) {
+ long clientVersion,
+ boolean
isAddingColumns,
+ List<PTable>
childViews) {
Review comment:
Do we need to add childViews to validateAndAddMetadata method ? Maybe I
am reading it wrong but I don't see childViews getting used either in
AddColumnMutator#validateAndAddMetadata or
DropColumnMutator#validateAndAddMetadata.
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/DropColumnMutator.java
##########
@@ -268,7 +272,19 @@ public MetaDataMutationResult
validateAndAddMetadata(PTable table,
}
}
- tableMetaData.addAll(additionalTableMetaData);
+ if (isDroppingColumns) {
Review comment:
The changes here looks exactly same as the changes in AddColumnMutator.
Can we create a helper method which accepts table and additionalTableMetaData
as argument ?
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java
##########
@@ -3063,60 +3063,62 @@ public boolean isViewReferenced() {
*/
EncodedCQCounter cqCounterToBe = tableType == PTableType.VIEW ?
NULL_COUNTER : cqCounter;
PTable table = new PTableImpl.Builder()
- .setType(tableType)
Review comment:
Here we are adding just 1 filed to builder method. Can we please undo
the formatting changes which is making the diff look more than actual changes.
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java
##########
@@ -107,6 +107,29 @@
HColumnDescriptor.KEEP_DELETED_CELLS,
HColumnDescriptor.REPLICATION_SCOPE);
+ public static Put getLastDDLTimestampUpdate(byte[] tableHeaderRowKey,
+ long clientTimestamp,
+ long lastDDLTimestamp) {
+ //use client timestamp as the timestamp of the Cell, to match the
other Cells that might
+ // be created by this DDL. But the actual value will be a _server_
timestamp
+ Put p = new Put(tableHeaderRowKey, clientTimestamp);
+ byte[] lastDDLTimestampBytes =
PLong.INSTANCE.toBytes(lastDDLTimestamp);
+ p.addColumn(PhoenixDatabaseMetaData.TABLE_FAMILY_BYTES,
+ PhoenixDatabaseMetaData.LAST_DDL_TIMESTAMP_BYTES,
lastDDLTimestampBytes);
+ return p;
+ }
+
+ /**
+ * Checks if a table is meant to be queried directly (and hence is
relevant to external
+ * systems tracking Phoenix schema)
+ * @param tableType
+ * @return True if a table or view, false otherwise (such as for an index,
system table, or
+ * subquery)
+ */
+ public static boolean isTableQueryable(PTableType tableType) {
Review comment:
I am not that much aware of the change here but isn't system table
directly queryable ?
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -2285,6 +2286,7 @@ public MetaDataResponse call(MetaDataService instance)
throws IOException {
builder.setClientVersion(VersionUtil.encodeVersion(PHOENIX_MAJOR_VERSION,
PHOENIX_MINOR_VERSION, PHOENIX_PATCH_NUMBER));
if (parentTable!=null)
builder.setParentTable(PTableImpl.toProto(parentTable));
+ builder.setAddingColumns(addingColumns);
Review comment:
Unable to understand the need of this field addingColumns in builder ?
If we need this, then why not a corresponding field in DropColumn request.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
> Store table metadata last modified timestamp in PTable / System.Catalog
> -----------------------------------------------------------------------
>
> Key: PHOENIX-6186
> URL: https://issues.apache.org/jira/browse/PHOENIX-6186
> Project: Phoenix
> Issue Type: New Feature
> Reporter: Geoffrey Jacoby
> Assignee: Geoffrey Jacoby
> Priority: Major
> Fix For: 4.16.0
>
> Attachments: PHOENIX-6186-4.x.patch
>
>
> There are many reasons why it's useful to know when a particular table's
> metadata was last modified. It's helpful when solving cache coherency
> problems, and also in order to interact with external schema registries which
> may have multiple versions of a particular schema and require a timestamp to
> resolve ambiguities.
> This JIRA will add a last modified timestamp field to System.Catalog, to be
> updated both when creating a table/view and also when adding or removing a
> column. Changing purely internal Phoenix properties will not update the
> timestamp.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
