PHOENIX-4893 Move parent column combining logic of view and view inde… #541
Conversation
twdsilva
force-pushed
the
PHOENIX-4893
branch
4 times, most recently
from
b156d58
to
d3bd6fc
Compare
|
@ChinmaySKulkarni Can you please review? |
|
|
||
| // Prior to PHOENIX-4766 we were sending the parent table column metadata while creating a |
There was a problem hiding this comment.
We cannot ignore parent table columns while building the PTable of a child view as its possible this call is from an older client. For a new client duplicate parent table columns will be resolved by picking the one with the latest timestamp.
| List<RowLock> locks = Lists.newArrayList(); | ||
| try { | ||
| if (expectedType == PTableType.TABLE) { | ||
| childViews = findAllChildViews(clientTimeStamp, tenantId, schemaName, tableName); |
There was a problem hiding this comment.
find the child views before locking the current parent table row.
| } | ||
| } finally { | ||
| releaseRowLocks(region,locks); | ||
| releaseRowLocks(region, locks); | ||
| // drop indexes on views that require the column being dropped |
There was a problem hiding this comment.
drop (possibly remote) child view indexes after releasing the parent table lock. If in the meantime one of these views are resolved, we will resolve the parent table and skip adding any indexes that can't be used.
| @@ -144,11 +141,13 @@ message DropSchemaRequest { | |||
| message AddColumnRequest { | |||
| repeated bytes tableMetadataMutations = 1; | |||
| optional int32 clientVersion = 2; | |||
| optional PTable parentTable = 3; | |||
There was a problem hiding this comment.
Need to send over the parent PTable while dropping or adding a column to a child view (so that we can construct the complete metadata on the server side)
| @@ -87,9 +87,6 @@ message GetTableRequest { | |||
| required int64 tableTimestamp = 4; | |||
| required int64 clientTimestamp = 5; | |||
| optional int32 clientVersion = 6; | |||
| optional bool skipAddingParentColumns = 7; | |||
| optional bool skipAddingIndexes = 8; | |||
| optional PTable lockedAncestorTable = 9; | |||
| } | |||
There was a problem hiding this comment.
No long need any of these since we aren't combining columns on the server side anymore.
| // if the update mutation caused tables to be deleted, the mutation code returned | ||
| // will be MutationCode.TABLE_ALREADY_EXISTS | ||
| if (result != null | ||
| && result.getMutationCode() != MutationCode.TABLE_ALREADY_EXISTS) { | ||
| return result; | ||
| } | ||
|
|
||
| // drop any indexes on the base table othat need the column that is going to be dropped |
There was a problem hiding this comment.
nit: typo "othat"
| // 4.15 onwards. | ||
| // Also if QueryServices.ALLOW_SPLITTABLE_SYSTEM_CATALOG_ROLLBACK is true, we block adding | ||
| // a column to a parent table so that we can rollback the upgrade if required. | ||
| if (clientVersion < MIN_SPLITTABLE_SYSTEM_CATALOG) { |
There was a problem hiding this comment.
So an older client can no longer add any columns to a base table if that table has child views?
There was a problem hiding this comment.
Yes because we no longer propagate metadata changes from parent to children, so in order to support rollback we need to prevent adding or dropping columns to a parent table.
| } catch (ClassNotFoundException e) { | ||
| } | ||
| for (PTable index : table.getIndexes()) { | ||
| // ignore any indexes dervied from ancestors |
There was a problem hiding this comment.
nit: typo "dervied"
| } catch (ClassNotFoundException e) { | ||
| } | ||
| for (PTable index : table.getIndexes()) { | ||
| // ignore any indexes dervied from ancestors |
There was a problem hiding this comment.
This will ignore indexes derived by grandchild views from their parents. Why do we want to skip them? What if these derived indexes use the column that is to be dropped?
There was a problem hiding this comment.
These inherited indexes are added to the ptable of a view in MetdataClient by resolving the parent table. After the column is dropped the next time the view is resolved the index would have already been dropped on the parent.
| return result; | ||
| } | ||
| // there should be no child links to delete since we are just dropping an index | ||
| assert (childLinksMutations.isEmpty()); |
There was a problem hiding this comment.
This can potentially bring down the region server if an AssertionError occurs. Is this check necessary? Can we log an error instead?
There was a problem hiding this comment.
good idea, will change this.
| @@ -424,6 +425,7 @@ public SQLException newException(SQLExceptionInfo info) { | |||
| CONCURRENT_UPGRADE_IN_PROGRESS(2010, "INT12", ""), | |||
| UPGRADE_REQUIRED(2011, "INT13", ""), | |||
| UPGRADE_NOT_REQUIRED(2012, "INT14", ""), | |||
| GET_TABLE_ERROR(2013, "INT15", "MetadataEndointImpl doGetTable called for table not present on region"), | |||
There was a problem hiding this comment.
nit: Typo "MetadataEndointImpl"
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.phoenix.coprocessor; |
There was a problem hiding this comment.
Can we move this to org.apache.phoenix.util since it's used by client and server-side code?
| @@ -1667,6 +1664,15 @@ public static PTable createFromProto(PTableProtos.PTable table) { | |||
| if (table.hasUseStatsForParallelization()) { | |||
| useStatsForParallelization = table.getUseStatsForParallelization(); | |||
| } | |||
| // for older clients just use the value of the propeties that are set on the view | |||
There was a problem hiding this comment.
nit: typo "propeties"
…xes from server to client
|
Thanks for the review @ChinmaySKulkarni |
…xes from server to client
This PR moves the column combining logic from the server to the client. For older clients since the child view metadata included the parent column metadata as well when we return the
PTablefrom the server it will already include parent table column metadata.I moved the add and drop column code in
MetadataEndpointImplthat was present in anonymous classes that implementedColumnMutatorinto their own classesAddColumnMutatorandDropColumnMutator.I also renamed
ViewFindertoViewUtiland moved the column combining logic fromMetadataEndpointImpland the code that adds indexes from parent tables fromMetadataClientto this util.