-
Notifications
You must be signed in to change notification settings - Fork 1k
PHOENIX-6942: Some config properties do not have phoenix prefix #1656
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
Conversation
6d88671 to
50710ae
Compare
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.
Can you add a test where you check the deprecation mechanism ?
Also check that we're not using these strings as Scan attributes (we'd need extra code to handle both versions)
| "|org.apache.phoenix.index.GlobalIndexChecker|805306365|"; | ||
| public static final String INDEX_REGION_OBSERVER_CONFIG = | ||
| "|org.apache.phoenix.hbase.index.IndexRegionObserver|805306366|" + | ||
| "index.builder=org.apache.phoenix.index.PhoenixIndexBuilder," + |
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.
index.builder and org.apache.hadoop.hbase.index.codec.class are present in the HBase table descriptor.
For data backwards compatibility reasons I'd prefer not change these two (even though they have the same problems as others).
i.e. Changing these would break older versions of Phoenix when trying to use them if we changed these.
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.
And these are not read from hbase-site.xml, but from the Hbase table metadata directly.
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.
Thank you for pointing this out, will restore the original strings.
The string "index.builder" was declared twice though, I think one of them was unnecessary.
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.
Can you revert the whitespace changes ?
If we don't change these, it's better to leave the file alone.
stoty
left a comment
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.
Some nits and whitespace issues
| "|org.apache.phoenix.index.GlobalIndexChecker|805306365|"; | ||
| public static final String INDEX_REGION_OBSERVER_CONFIG = | ||
| "|org.apache.phoenix.hbase.index.IndexRegionObserver|805306366|" + | ||
| "index.builder=org.apache.phoenix.index.PhoenixIndexBuilder," + |
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.
Can you revert the whitespace changes ?
If we don't change these, it's better to leave the file alone.
|
|
||
| private static final int DEFAULT_ROWLOCK_WAIT_DURATION = 30000; | ||
| private static final int DEFAULT_CONCURRENT_MUTATION_WAIT_DURATION_IN_MS = 100; | ||
|
|
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.
nit: don't delete the empty line
| .encodeVersion("0.94.9"); | ||
|
|
||
| private static final int DEFAULT_ROWLOCK_WAIT_DURATION = 30000; | ||
|
|
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.
nit: don't delete the empty line
stoty
left a comment
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 LGTM
No description provided.