PHOENIX-4009 Run UPDATE STATISTICS command by using MR integration on… #433
Conversation
… snapshots (Addendum)
|
Here's a brief insight on the addendum PR. Others are mostly code refactoring changes and package name changes. |
| @@ -464,6 +464,8 @@ public SQLException newException(SQLExceptionInfo info) { | |||
| INSUFFICIENT_MEMORY(999, "50M01", "Unable to allocate enough memory."), | |||
| HASH_JOIN_CACHE_NOT_FOUND(900, "HJ01", "Hash Join cache not found"), | |||
|
|
|||
| STATS_COLLECTION_DISABLED_ON_SERVER(1401, "STS01", "Stats collection attempted but is disabled on server"), | |||
There was a problem hiding this comment.
In general the error codes appear to be grouped by number/purpose. Do we have system level error code or stats codes? Do we want to reserve a prefix range for this 14XX? Consider moving to a different location for sorting reasons.
There was a problem hiding this comment.
In general the error codes appear to be grouped by number/purpose. Do we have system level error code or stats codes?
Yes thats correct, there are some conventions. I am not sure if these are followed here. Phoenix has a bunch of its own exception types not really relevant to SQL.
Do we want to reserve a prefix range for this 14XX?
I am not sure if we can do that and if yes, whats the way to do that.
Consider moving to a different location for sorting reasons
Will do
There was a problem hiding this comment.
@twdsilva Are you aware of any conventions we are trying to hold for SQL exceptions?
There was a problem hiding this comment.
I'm very confusion about the naming convention we used for the first two fields, eg., "1401" and "STS01". The last 4 sql exceptions defined are totally out of orders which mightn't be good.
There was a problem hiding this comment.
The exceptions are not sorted in the code. The 1401 and STS01 are arbitrarily chosen. Not sure about the next 4 exceptions.
There was a problem hiding this comment.
Spoke with Thomas offline, he is okay with this. We are not really following SQL conventions here.
| /** | ||
| * Tests the behavior of stats collection code when stats are disabled on server side | ||
| */ | ||
| public class NoOpStatsCollectorIT extends ParallelStatsDisabledIT { |
There was a problem hiding this comment.
This class tests a bunch of negative cases. Do we have good coverage on the positive cases such that we are confident the removal of STATS_ENABLED_ATTRIB does not break anything?
There was a problem hiding this comment.
The property was deprecated more than 2 releases ago. As of now I have removed its total usage.
I don't think that it would break anything.
| @@ -70,8 +69,7 @@ public GuidePostsCache(ConnectionQueryServices queryServices, Configuration conf | |||
| final long maxTableStatsCacheSize = config.getLong( | |||
| QueryServices.STATS_MAX_CACHE_SIZE, | |||
| QueryServicesOptions.DEFAULT_STATS_MAX_CACHE_SIZE); | |||
| final boolean isStatsEnabled = config.getBoolean(STATS_COLLECTION_ENABLED, DEFAULT_STATS_COLLECTION_ENABLED) | |||
| && config.getBoolean(STATS_ENABLED_ATTRIB, true); | |||
| final boolean isStatsEnabled = config.getBoolean(STATS_COLLECTION_ENABLED, DEFAULT_STATS_COLLECTION_ENABLED); | |||
| import java.sql.SQLException; | ||
|
|
||
| public class StatsCollectionDisabledOnServerException extends SQLException { | ||
|
|
There was a problem hiding this comment.
remove empty line here?
| private static final long serialVersionUID = 1L; | ||
| private static SQLExceptionCode code = SQLExceptionCode.STATS_COLLECTION_DISABLED_ON_SERVER; | ||
|
|
||
| public StatsCollectionDisabledOnServerException() { |
There was a problem hiding this comment.
It's better to pass table name and call setTableNae in SQLExceptionInfo.Builder.
There was a problem hiding this comment.
The exception is wrapped around by DoNotRetryIOException from HBase which prints the region name automatically (which contains table name), hence I didn't add it here.
|
Thanks @BinShi-SecularBird and @dbwong for the review. |
… snapshots (Addendum) (apache#433)
… snapshots (Addendum)