Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6125 : Disable region split for SYSTEM.TASK - #923
Conversation
stoty
commented
Oct 15, 2020
💔 -1 overall
This message was automatically generated. |
stoty
commented
Oct 15, 2020
💔 -1 overall
This message was automatically generated. |
ChinmaySKulkarni
left a comment
There was a problem hiding this comment.
Added some comments. Also, the test failure seems related since it is complaining about TableNotFoundException for SYSTEM.TASK
| .getRegionSplitPolicyClassName(); | ||
| assertEquals(DisabledRegionSplitPolicy.class.getName(), | ||
| taskSplitPolicy); | ||
| } |
There was a problem hiding this comment.
Do we really need a separate IT for this? There is an existing IT that uses the upgrade counting driver and services. Let's reuse that instead
| } | ||
| private boolean isSplitPolicyUpdatedForTaskTable( |
There was a problem hiding this comment.
Since this method actually also replaces the split policy, we should rename it to indicate that.
| private boolean isSplitPolicyUpdatedForTaskTable( | ||
| final TableDescriptorBuilder tdBuilder) { |
There was a problem hiding this comment.
Can we add a new unit test for this method?
| try { | ||
| tableName = TableName.valueOf(tableNameStr); | ||
| td = admin.getDescriptor(tableName); | ||
| } catch (org.apache.hadoop.hbase.TableNotFoundException tnfe) { |
There was a problem hiding this comment.
Same question here, can we not use namespace mapping property to ensure we are looking for the correct name '.' vs ':'?
Uh oh!
There was an error while loading. Please reload this page.
| if (tdBuilder.build().getTableName().getNameAsString() | ||
| .equals(taskTable)) { | ||
| isTaskTable = true; | ||
| } else { |
There was a problem hiding this comment.
Can't we use the actual namespace mapping property to get the exact expected name i.e. SYSTEM.TASK vs SYSTEM:TASK instead of trying both variations?
| newDesc.setRegionSplitPolicyClassName(IndexRegionSplitPolicy.class.getName()); | ||
| } | ||
| // disable split policy for SYSTEM.TASK | ||
| isSplitPolicyUpdatedForTaskTable(newDesc); |
There was a problem hiding this comment.
All table creations go through this method so instead of polluting this with SYSTEM.TASK specific stuff, I think we should divide the problem into 2 parts:
Fresh clusters and <4.15 clusters -> Change the DDL for SYSTEM.TASK (CQSI.CREATE_TASK_METADATA) itself so this would work directly i.e. add
HTableDescriptor.SPLIT_POLICY + "='" + SystemTaskSplitPolicy.class.getName() + "',\n"
and introduce a new classSystemTaskSplitPolicywhich is equivalent toDisabledRegionSplitPolicyfor now, but can be easily changed in the future if required without unloading and reloading the coproc.Upgrade path for 4.15 clusters -> We would go through upgradeSystemTask() and here we could load the correct split policy. That way, we don't need the change inside ensureTableCreated() and it is much cleaner. One corner case here (highly unlikely, but still possible) is if a cluster already has SYSTEM.TASK split. In that case, we might want to throw an exception or maybe merge regions of SYSTEM.TASK
Uh oh!
There was an error while loading. Please reload this page.
virajjasani
commented
Oct 16, 2020
Thanks @ChinmaySKulkarni . Addressed concerns and updated PR accordingly. Test failure is also taken care of. |
stoty
commented
Oct 16, 2020
💔 -1 overall
This message was automatically generated. |
stoty
commented
Oct 16, 2020
💔 -1 overall
This message was automatically generated. |
stoty
commented
Oct 19, 2020
💔 -1 overall
This message was automatically generated. |
| countUpgradeAttempts++; | ||
| } | ||
There was a problem hiding this comment.
These changes are specific only to whitespace issues that precommit build reported.
stoty
commented
Oct 19, 2020
💔 -1 overall
This message was automatically generated. |
ChinmaySKulkarni
left a comment
There was a problem hiding this comment.
+1 thanks @virajjasani I will commit this to master and 4.x today
ChinmaySKulkarni
commented
Oct 19, 2020
@virajjasani the master patch doesn't apply cleanly to 4.x so please put up a patch/PR for 4.x, thanks |
virajjasani
commented
Oct 19, 2020
Done, uploaded patch on Jira. Thanks for the review @ChinmaySKulkarni |
No description provided.