[
https://issues.apache.org/jira/browse/PHOENIX-1409?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14259473#comment-14259473
]
James Taylor commented on PHOENIX-1409:
---------------------------------------
Nice work, [~samarthjain] and [~ayingshu] - thanks for the tag team on this.
Here's some feedback:
- For the new modifyTable API in ConnectionQueryServices, don't include
HBaseAdmin as an argument. Instead, you can get an HBaseAdmin inside the method
implementation in ConnectionQueryServicesImpl and then just delegate to the
existing private modifyTable method.
{code}
+ @Override
+ public void modifyTable(byte[] tableName, HBaseAdmin admin,
HTableDescriptor newDesc) throws IOException,
{code}
- I think a good follow up JIRA would be to have a new config property that
specifies whether or not non HBase properties may be set on the
HTableDescriptor. This is what we do today, if a property is not a Phoenix
property, nor a known HColumnDescriptor property, it ends up being put on the
HTableDescriptor. BTW - is this still the case and if we don't have a test for
this, would you mind adding one, [~samarthjain]? This is handy if you have your
own application specific properties, but not handy if you don't need this (and
have a typo in your DDL statement, as you won't know). So if this new config
property is false, we'd throw this exception:
{code}
+ // invalid property - neither of HTableProp,
HColumnProp or PhoenixTableProp
+ // FIXME: This isn't getting triggered as
currently a property gets evaluated
+ // as HTableProp if its neither HColumnProp or
PhoenixTableProp.
+ throw new
SQLExceptionInfo.Builder(SQLExceptionCode.SET_UNSUPPORTED_PROP_ON_ALTER_TABLE)
+ .setMessage("Column Family: " + family + ",
Property: " + propName).build()
+ .buildException();
{code}
- Small optimization: how about if modifyTableProps returns the
HTableDescriptor so in the case that it's called, you don't need to get the
HTableDescriptor again for setTTLForNewColumnFamilies?
{code}
+ if (!newColumnFamiliesToBeAdded.isEmpty()) {
+ // We make sure that all the CFs of the table have the
same TTL as the empty CF.
+ // We ensure this by disallowing column family specific
TTLs.
+ // For the new column families being added, we need to
make sure that their
+ // TTLs are the same as the TTL of the empty CF.
+ setTTLForNewColumnFamilies(newColumnFamiliesToBeAdded,
allFamiliesProps, table, emptyCF);
+ }
+
+ if (!tableProps.isEmpty()) {
+ modifyTableProps(tableName, table, tableProps);
+ }
+
{code}
- For the new Property class, define the PropertyValidator.validate method like
this instead, as you can get the propertyName, isMutable, and isValidOnView
from the TableProperty. Another perhaps even simpler approach would be to
define a validate method on the TableProperty enum. Also, instead of passing
through familyName (which you don't really need), you could pass through a
boolean like isSettingOnColumnFamily. Or do this check outside of the validate,
as it's never valid to have a column family set for a TableProperty, right? You
can reserve the validate method for property-specific checks instead.
{code}
void validate(TableProperty property, boolean isMutating, String familyName,
PTableType tableType) throws SQLException;
{code}
- Regarding this change, it might be there specifically for when you're running
the unit tests against a real cluster (is that correct [~jeffreyz]?), so you
may want to conditionally add this shutdown hook if that's the case.
{code}
diff --git a/phoenix-core/src/test/java/org/apache/phoenix/query/BaseTest.java
b/phoenix-core/src/test/java/org/apache/phoenix/query/BaseTest.java
index 313fc46..2f1d8d9 100644
--- a/phoenix-core/src/test/java/org/apache/phoenix/query/BaseTest.java
+++ b/phoenix-core/src/test/java/org/apache/phoenix/query/BaseTest.java
@@ -549,17 +549,6 @@ public abstract class BaseTest {
utility = new HBaseTestingUtility(conf);
try {
utility.startMiniCluster();
- // add shutdown hook to kill the mini cluster
- Runtime.getRuntime().addShutdownHook(new Thread() {
- @Override
- public void run() {
- try {
- if (utility != null) utility.shutdownMiniCluster();
- } catch (Exception e) {
- logger.warn("Exception caught when shutting down mini
cluster", e);
- }
- }
- });
return getLocalClusterUrl(utility);
} catch (Throwable t) {
throw new RuntimeException(t);
{code}
+1 after these changes.
> Allow ALTER TABLE <table> SET command to update HTableDescriptor and
> HColumnDescriptor properties
> -------------------------------------------------------------------------------------------------
>
> Key: PHOENIX-1409
> URL: https://issues.apache.org/jira/browse/PHOENIX-1409
> Project: Phoenix
> Issue Type: Improvement
> Affects Versions: 4.2
> Reporter: James Taylor
> Assignee: Alicia Ying Shu
> Attachments: PHOENIX-1409-v3.patch, Phoenix-1409-v1.patch,
> Phoenix-1409.patch, WIP.patch, phoenix-1409-v2.patch
>
>
> Once PHOENIX-1408 is fixed, we should allow HTableDescriptor and
> HColumnDescriptor properties through the ALTER TABLE <table> SET command.
> It'd just be a matter of passing these properties through the existing
> methods, as we support this for CREATE TABLE.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
