You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2020/10/14 11:43:14 UTC
[GitHub] [phoenix] virajjasani opened a new pull request #920: PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table
virajjasani opened a new pull request #920:
URL: https://github.com/apache/phoenix/pull/920
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] richardantal commented on a change in pull request #920: PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table
Posted by GitBox <gi...@apache.org>.
richardantal commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r504753124
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4294,11 +4295,9 @@ public boolean acquireUpgradeMutex(long currentServerSideTableTimestamp)
public boolean writeMutexCell(String tenantId, String schemaName, String tableName,
String columnName, String familyName) throws SQLException {
try {
- byte[] rowKey =
- columnName != null
- ? SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName,
- familyName)
- : SchemaUtil.getTableKey(tenantId, schemaName, tableName);
+ byte[] rowKey = columnName != null ?
+ SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, familyName) :
+ SchemaUtil.getTableKey(tenantId, schemaName, tableName);
// at this point the system mutex table should have been created or
// an exception thrown
byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes();
Review comment:
`try (Table sysMutexTable = getTable(sysMutexPhysicalTableNameBytes)) {`
Instead We could try the Table sysMutexTable =getTable() call with one of them and catch HBase TableNotFoundException, in that case try with the other one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #920: PHOENIX-6129 : Optimize tableExists() call while retrieving correct MUTEX table
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #920:
URL: https://github.com/apache/phoenix/pull/920#issuecomment-714596384
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 2m 0s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 15m 15s | master passed |
| +1 :green_heart: | compile | 1m 7s | master passed |
| +1 :green_heart: | checkstyle | 1m 9s | master passed |
| +1 :green_heart: | javadoc | 0m 51s | master passed |
| +0 :ok: | spotbugs | 3m 20s | phoenix-core in master has 970 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 11m 0s | the patch passed |
| +1 :green_heart: | compile | 1m 3s | the patch passed |
| +1 :green_heart: | javac | 1m 3s | the patch passed |
| -1 :x: | checkstyle | 1m 9s | phoenix-core: The patch generated 3 new + 2223 unchanged - 21 fixed = 2226 total (was 2244) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 50s | the patch passed |
| +1 :green_heart: | spotbugs | 3m 35s | phoenix-core generated 0 new + 969 unchanged - 1 fixed = 969 total (was 970) |
||| _ Other Tests _ |
| -1 :x: | unit | 152m 0s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 22s | The patch does not generate ASF License warnings. |
| | | 196m 10s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | phoenix.end2end.OrphanViewToolIT |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/8/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/920 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux 962ce30e7af0 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | master / 8c81131 |
| Default Java | Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/8/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/8/artifact/yetus-general-check/output/patch-unit-phoenix-core.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/8/testReport/ |
| Max. process+thread count | 6202 (vs. ulimit of 30000) |
| modules | C: phoenix-core U: phoenix-core |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/8/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #920: PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #920:
URL: https://github.com/apache/phoenix/pull/920#issuecomment-709965235
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 2m 43s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| -1 :x: | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
||| _ master Compile Tests _ |
| -1 :x: | mvninstall | 45m 37s | root in master failed. |
| +1 :green_heart: | compile | 1m 3s | master passed |
| +1 :green_heart: | checkstyle | 1m 4s | master passed |
| +1 :green_heart: | javadoc | 0m 49s | master passed |
| +0 :ok: | spotbugs | 3m 16s | phoenix-core in master has 973 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| -1 :x: | mvninstall | 42m 40s | root in the patch failed. |
| +1 :green_heart: | compile | 1m 10s | the patch passed |
| +1 :green_heart: | javac | 1m 10s | the patch passed |
| -1 :x: | checkstyle | 1m 10s | phoenix-core: The patch generated 31 new + 2192 unchanged - 50 fixed = 2223 total (was 2242) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 58s | the patch passed |
| +1 :green_heart: | spotbugs | 4m 2s | phoenix-core generated 0 new + 972 unchanged - 1 fixed = 972 total (was 973) |
||| _ Other Tests _ |
| -1 :x: | unit | 155m 13s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 22s | The patch does not generate ASF License warnings. |
| | | 262m 36s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | phoenix.end2end.SystemTablesCreationOnConnectionIT |
| | phoenix.end2end.MigrateSystemTablesToSystemNamespaceIT |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/3/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/920 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux eb4521000838 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | master / 628fa0d |
| Default Java | Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08 |
| mvninstall | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/3/artifact/yetus-general-check/output/branch-mvninstall-root.txt |
| mvninstall | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/3/artifact/yetus-general-check/output/patch-mvninstall-root.txt |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/3/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/3/artifact/yetus-general-check/output/patch-unit-phoenix-core.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/3/testReport/ |
| Max. process+thread count | 6207 (vs. ulimit of 30000) |
| modules | C: phoenix-core U: phoenix-core |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/3/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #920: PHOENIX-6129 : Optimize tableExists() call while retrieving correct MUTEX table
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #920:
URL: https://github.com/apache/phoenix/pull/920#issuecomment-713082959
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 5m 45s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| -1 :x: | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 14m 44s | master passed |
| +1 :green_heart: | compile | 1m 2s | master passed |
| +1 :green_heart: | checkstyle | 1m 6s | master passed |
| +1 :green_heart: | javadoc | 0m 48s | master passed |
| +0 :ok: | spotbugs | 3m 9s | phoenix-core in master has 973 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 9m 4s | the patch passed |
| +1 :green_heart: | compile | 0m 57s | the patch passed |
| +1 :green_heart: | javac | 0m 57s | the patch passed |
| -1 :x: | checkstyle | 1m 8s | phoenix-core: The patch generated 31 new + 2191 unchanged - 50 fixed = 2222 total (was 2241) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 47s | the patch passed |
| +1 :green_heart: | spotbugs | 3m 19s | phoenix-core generated 0 new + 972 unchanged - 1 fixed = 972 total (was 973) |
||| _ Other Tests _ |
| -1 :x: | unit | 164m 43s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 24s | The patch does not generate ASF License warnings. |
| | | 209m 18s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | phoenix.end2end.TableSnapshotReadsMapReduceIT |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/6/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/920 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux 8835fa318632 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | master / 4703e46 |
| Default Java | Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/6/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/6/artifact/yetus-general-check/output/patch-unit-phoenix-core.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/6/testReport/ |
| Max. process+thread count | 6239 (vs. ulimit of 30000) |
| modules | C: phoenix-core U: phoenix-core |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/6/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ChinmaySKulkarni commented on a change in pull request #920: PHOENIX-6129 : Optimize tableExists() call while retrieving correct MUTEX table
Posted by GitBox <gi...@apache.org>.
ChinmaySKulkarni commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r512142414
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4364,17 +4353,17 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName
}
}
- private byte[] getSysMutexPhysicalTableNameBytes() throws IOException, SQLException {
- byte[] sysMutexPhysicalTableNameBytes = null;
- try(Admin admin = getAdmin()) {
- if(admin.tableExists(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME)) {
- sysMutexPhysicalTableNameBytes = PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME_BYTES;
- } else if (admin.tableExists(TableName.valueOf(
- SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName()))) {
- sysMutexPhysicalTableNameBytes = SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName();
+ private Table getSysMutexTable() throws SQLException, IOException {
+ String table = SYSTEM_MUTEX_NAME;
+ TableName tableName = TableName.valueOf(table);
+ try (Admin admin = getAdmin()) {
+ if (!admin.tableExists(tableName)) {
+ table = table.replace(QueryConstants.NAME_SEPARATOR,
+ QueryConstants.NAMESPACE_SEPARATOR);
+ tableName = TableName.valueOf(table);
}
+ return connection.getTable(tableName);
Review comment:
Got it. Makes sense
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ChinmaySKulkarni commented on a change in pull request #920: PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table
Posted by GitBox <gi...@apache.org>.
ChinmaySKulkarni commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r505900118
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4294,11 +4295,9 @@ public boolean acquireUpgradeMutex(long currentServerSideTableTimestamp)
public boolean writeMutexCell(String tenantId, String schemaName, String tableName,
String columnName, String familyName) throws SQLException {
try {
- byte[] rowKey =
- columnName != null
- ? SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName,
- familyName)
- : SchemaUtil.getTableKey(tenantId, schemaName, tableName);
+ byte[] rowKey = columnName != null ?
+ SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, familyName) :
+ SchemaUtil.getTableKey(tenantId, schemaName, tableName);
// at this point the system mutex table should have been created or
// an exception thrown
byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes();
Review comment:
@virajjasani I think there is some confusion here. The aim of this Jira is to reduce the HBase calls to get the table. Currently, the call to `writeMutexCell()` calls `getSysMutexPhysicalTableNameBytes()` which does 1 or 2 HBase admin calls (tableExists()) and then we still do a getTable() call [here](https://github.com/apache/phoenix/blob/628fa0db239985b2972f51459540f7513d182885/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java#L4305).
The same happens for `deleteMutexCell()`.
Instead of calling `getSysMutexPhysicalTableNameBytes()`, we can do 1 getTable call with `SYSTEM.MUTEX` and if that throws a TNFE, try again with `SYSTEM:MUTEX` thus eliminating the `tableExists()` calls.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #920: PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #920:
URL: https://github.com/apache/phoenix/pull/920#issuecomment-709267634
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 10m 18s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| -1 :x: | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 49m 54s | master passed |
| +1 :green_heart: | compile | 1m 15s | master passed |
| +1 :green_heart: | checkstyle | 1m 17s | master passed |
| +1 :green_heart: | javadoc | 0m 57s | master passed |
| +0 :ok: | spotbugs | 3m 23s | phoenix-core in master has 973 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 41m 32s | the patch passed |
| +1 :green_heart: | compile | 0m 59s | the patch passed |
| +1 :green_heart: | javac | 0m 59s | the patch passed |
| -1 :x: | checkstyle | 1m 8s | phoenix-core: The patch generated 39 new + 2201 unchanged - 41 fixed = 2240 total (was 2242) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 45s | the patch passed |
| +1 :green_heart: | spotbugs | 3m 20s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 166m 17s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 22s | The patch does not generate ASF License warnings. |
| | | 283m 57s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | phoenix.end2end.OrphanViewToolIT |
| | phoenix.end2end.ConcurrentMutationsExtendedIT |
| | phoenix.end2end.ViewMetadataIT |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/920 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux 747e6da3f8ca 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | master / 5b2eeb6 |
| Default Java | Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/1/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/1/artifact/yetus-general-check/output/patch-unit-phoenix-core.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/1/testReport/ |
| Max. process+thread count | 6223 (vs. ulimit of 30000) |
| modules | C: phoenix-core U: phoenix-core |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/1/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani commented on a change in pull request #920: PHOENIX-6129 : Optimize tableExists() call while retrieving correct MUTEX table
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r509509727
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4364,17 +4353,17 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName
}
}
- private byte[] getSysMutexPhysicalTableNameBytes() throws IOException, SQLException {
- byte[] sysMutexPhysicalTableNameBytes = null;
- try(Admin admin = getAdmin()) {
- if(admin.tableExists(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME)) {
- sysMutexPhysicalTableNameBytes = PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME_BYTES;
- } else if (admin.tableExists(TableName.valueOf(
- SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName()))) {
- sysMutexPhysicalTableNameBytes = SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName();
+ private Table getSysMutexTable() throws SQLException, IOException {
+ String table = SYSTEM_MUTEX_NAME;
+ TableName tableName = TableName.valueOf(table);
+ try (Admin admin = getAdmin()) {
+ if (!admin.tableExists(tableName)) {
+ table = table.replace(QueryConstants.NAME_SEPARATOR,
+ QueryConstants.NAMESPACE_SEPARATOR);
+ tableName = TableName.valueOf(table);
}
+ return connection.getTable(tableName);
Review comment:
> This scenario is the same as returning null from the previous getSysMutexPhysicalTableNameBytes() method and then attempting to retrieve a null table.
Basically, we are certain that either `SYSTEM.MUTEX` or `SYSTEM:MUTEX` exist at any point in time right? e.g if we are performing namespace upgrade (e.g MigrateSystemTablesToSystemNamespaceIT test), when SYSTEM.MUTEX no longer exists, if we call `HBaseFactoryProvider.getHTableFactory().getTable()`, it does return `Table` object, it doesn't return null as such. However, using that table object, we can't perform any operation because actual table would not exist.
As for this comment, the scenario is not same as previous `getSysMutexPhysicalTableNameBytes()` because when we reach at this point, based on above `Admin.tableExists()` call, we would call `connection.getTable()` with correct table name only (I hope we never have situation where SYSTEM.MUTEX and SYSTEM:MUTEX both are dropped. Should we really worry about this case?)
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4364,17 +4353,17 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName
}
}
- private byte[] getSysMutexPhysicalTableNameBytes() throws IOException, SQLException {
- byte[] sysMutexPhysicalTableNameBytes = null;
- try(Admin admin = getAdmin()) {
- if(admin.tableExists(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME)) {
- sysMutexPhysicalTableNameBytes = PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME_BYTES;
- } else if (admin.tableExists(TableName.valueOf(
- SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName()))) {
- sysMutexPhysicalTableNameBytes = SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName();
+ private Table getSysMutexTable() throws SQLException, IOException {
+ String table = SYSTEM_MUTEX_NAME;
+ TableName tableName = TableName.valueOf(table);
+ try (Admin admin = getAdmin()) {
+ if (!admin.tableExists(tableName)) {
+ table = table.replace(QueryConstants.NAME_SEPARATOR,
+ QueryConstants.NAMESPACE_SEPARATOR);
+ tableName = TableName.valueOf(table);
}
+ return connection.getTable(tableName);
Review comment:
On high level, `connection.getTable()` will never return `null`, it will return new `Table` object even for non existing tables, so we won't encounter NPE due to this for sure. However, I believe we should make just one API call `admin.tableExists()` so that we will know either `SYSTEM.MUTEX` or `SYSTEM:MUTEX` exist at any time. Sounds good?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani commented on a change in pull request #920: PHOENIX-6129 : Optimize tableExists() call while retrieving correct MUTEX table
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r509600822
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4364,17 +4353,17 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName
}
}
- private byte[] getSysMutexPhysicalTableNameBytes() throws IOException, SQLException {
- byte[] sysMutexPhysicalTableNameBytes = null;
- try(Admin admin = getAdmin()) {
- if(admin.tableExists(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME)) {
- sysMutexPhysicalTableNameBytes = PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME_BYTES;
- } else if (admin.tableExists(TableName.valueOf(
- SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName()))) {
- sysMutexPhysicalTableNameBytes = SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName();
+ private Table getSysMutexTable() throws SQLException, IOException {
+ String table = SYSTEM_MUTEX_NAME;
Review comment:
It's being used here:
```
table = table.replace(QueryConstants.NAME_SEPARATOR,
QueryConstants.NAMESPACE_SEPARATOR);
```
Is that fine?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ChinmaySKulkarni commented on a change in pull request #920: PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table
Posted by GitBox <gi...@apache.org>.
ChinmaySKulkarni commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r505900118
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4294,11 +4295,9 @@ public boolean acquireUpgradeMutex(long currentServerSideTableTimestamp)
public boolean writeMutexCell(String tenantId, String schemaName, String tableName,
String columnName, String familyName) throws SQLException {
try {
- byte[] rowKey =
- columnName != null
- ? SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName,
- familyName)
- : SchemaUtil.getTableKey(tenantId, schemaName, tableName);
+ byte[] rowKey = columnName != null ?
+ SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, familyName) :
+ SchemaUtil.getTableKey(tenantId, schemaName, tableName);
// at this point the system mutex table should have been created or
// an exception thrown
byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes();
Review comment:
@virajjasani I think there is some confusion here. The aim of this Jira is to reduce the HBase calls to get the table. Currently, the call to `writeMutexCell()` calls `getSysMutexPhysicalTableNameBytes()` which does 1 or 2 HBase admin calls (tableExists()) and then we still do a getTable() call [here](https://github.com/apache/phoenix/blob/628fa0db239985b2972f51459540f7513d182885/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java#L4305).
The same happens for `deleteMutexCell()`.
Instead of doing this, we can do 1 getTable call with `SYSTEM.MUTEX` and if that throws a TNFE, try again with `SYSTEM:MUTEX`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani commented on a change in pull request #920: PHOENIX-6129 : Optimize tableExists() call while retrieving correct MUTEX table
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r509600822
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4364,17 +4353,17 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName
}
}
- private byte[] getSysMutexPhysicalTableNameBytes() throws IOException, SQLException {
- byte[] sysMutexPhysicalTableNameBytes = null;
- try(Admin admin = getAdmin()) {
- if(admin.tableExists(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME)) {
- sysMutexPhysicalTableNameBytes = PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME_BYTES;
- } else if (admin.tableExists(TableName.valueOf(
- SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName()))) {
- sysMutexPhysicalTableNameBytes = SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName();
+ private Table getSysMutexTable() throws SQLException, IOException {
+ String table = SYSTEM_MUTEX_NAME;
Review comment:
It's being used here also, hence kept it for simplicity:
```
table = table.replace(QueryConstants.NAME_SEPARATOR,
QueryConstants.NAMESPACE_SEPARATOR);
```
Is that fine keeping `table` as is? It might look simplified. Thought?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani commented on pull request #920: PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #920:
URL: https://github.com/apache/phoenix/pull/920#issuecomment-710038097
@ChinmaySKulkarni IMHO we can not live without `tableExists()` call because `Connection.getTable()` has no guarantee that it will throw `TableNotFoundException` if table doesn't exist. What happens during namespace upgrade is that even if `SYSTEM.MUTEX` has been deleted (removed entry from meta), `Connection.getTable()` still returns us an instance of `Table`, however, we can't perform actions on it.
Admin.tableExists is the only thread safe way to identify if table already exists:
```
/**
* @param tableName Table to check.
* @return <code>true</code> if table exists already.
* @throws IOException if a remote or network exception occurs
*/
boolean tableExists(TableName tableName) throws IOException;
```
&
```
/**
* Retrieve a Table implementation for accessing a table.
* The returned Table is not thread safe, a new instance should be created for each using thread.
* This is a lightweight operation, pooling or caching of the returned Table
* is neither required nor desired.
* <p>
* The caller is responsible for calling {@link Table#close()} on the returned
* table instance.
* <p>
* Since 0.98.1 this method no longer checks table existence. An exception
* will be thrown if the table does not exist only when the first operation is
* attempted.
* @param tableName the name of the table
* @return a Table to use for interactions with this table
*/
default Table getTable(TableName tableName) throws IOException {
return getTable(tableName, null);
}
```
I checked implementation also, and getTable() internally uses `getTableBuilder()` which just provides a builder instance for just creating `Table` object to access actual table, but there is no guarantee that a table exists while accessing it. Only existsTable() goes to meta table and checks for the existing entry of Table.
We can do one improvement though, we can utilize same connection to perform both: tableExists() and getTable().
If we don't have `tableExists()`, test cases `MigrateSystemTablesToSystemNamespaceIT` are guaranteed to fail.
Updating the PR accordingly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani commented on a change in pull request #920: PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r504759424
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4294,11 +4295,9 @@ public boolean acquireUpgradeMutex(long currentServerSideTableTimestamp)
public boolean writeMutexCell(String tenantId, String schemaName, String tableName,
String columnName, String familyName) throws SQLException {
try {
- byte[] rowKey =
- columnName != null
- ? SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName,
- familyName)
- : SchemaUtil.getTableKey(tenantId, schemaName, tableName);
+ byte[] rowKey = columnName != null ?
+ SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, familyName) :
+ SchemaUtil.getTableKey(tenantId, schemaName, tableName);
// at this point the system mutex table should have been created or
// an exception thrown
byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes();
Review comment:
If `getSysMutexPhysicalTableNameBytes()` start throwing `TableNotFoundException`, then at this point the Exception would be thrown and caught, so we would not go ahead with next `getTable()` call.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani edited a comment on pull request #920: PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table
Posted by GitBox <gi...@apache.org>.
virajjasani edited a comment on pull request #920:
URL: https://github.com/apache/phoenix/pull/920#issuecomment-709339348
Test failures don't seem relevant, tried locally, all passed. Fixed checkstyle issues relevant to this PR.
Could you please take a look @yanxinyi ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #920: PHOENIX-6129 : Use same connection for tableExists() and getTable() in same method to retrieve correct MUTEX table
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #920:
URL: https://github.com/apache/phoenix/pull/920#issuecomment-710279269
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 1s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 1s | The patch does not contain any @author tags. |
| -1 :x: | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
||| _ master Compile Tests _ |
| -1 :x: | mvninstall | 46m 12s | root in master failed. |
| +1 :green_heart: | compile | 1m 3s | master passed |
| +1 :green_heart: | checkstyle | 1m 6s | master passed |
| +1 :green_heart: | javadoc | 0m 48s | master passed |
| +0 :ok: | spotbugs | 3m 7s | phoenix-core in master has 973 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| -1 :x: | mvninstall | 42m 31s | root in the patch failed. |
| +1 :green_heart: | compile | 0m 58s | the patch passed |
| +1 :green_heart: | javac | 0m 58s | the patch passed |
| -1 :x: | checkstyle | 1m 7s | phoenix-core: The patch generated 2 new + 2221 unchanged - 21 fixed = 2223 total (was 2242) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 46s | the patch passed |
| +1 :green_heart: | spotbugs | 3m 20s | phoenix-core generated 0 new + 972 unchanged - 1 fixed = 972 total (was 973) |
||| _ Other Tests _ |
| -1 :x: | unit | 150m 24s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 21s | The patch does not generate ASF License warnings. |
| | | 255m 8s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/4/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/920 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux 88e6e2e470a4 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | master / 628fa0d |
| Default Java | Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08 |
| mvninstall | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/4/artifact/yetus-general-check/output/branch-mvninstall-root.txt |
| mvninstall | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/4/artifact/yetus-general-check/output/patch-mvninstall-root.txt |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/4/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/4/artifact/yetus-general-check/output/patch-unit-phoenix-core.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/4/testReport/ |
| Max. process+thread count | 6092 (vs. ulimit of 30000) |
| modules | C: phoenix-core U: phoenix-core |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/4/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #920: PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #920:
URL: https://github.com/apache/phoenix/pull/920#issuecomment-709502608
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 19s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| -1 :x: | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 47m 16s | master passed |
| +1 :green_heart: | compile | 1m 4s | master passed |
| +1 :green_heart: | checkstyle | 1m 3s | master passed |
| +1 :green_heart: | javadoc | 0m 51s | master passed |
| +0 :ok: | spotbugs | 3m 37s | phoenix-core in master has 973 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 43m 2s | the patch passed |
| +1 :green_heart: | compile | 0m 57s | the patch passed |
| +1 :green_heart: | javac | 0m 57s | the patch passed |
| -1 :x: | checkstyle | 1m 6s | phoenix-core: The patch generated 4 new + 2230 unchanged - 12 fixed = 2234 total (was 2242) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 46s | the patch passed |
| +1 :green_heart: | spotbugs | 3m 18s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 156m 12s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 21s | The patch does not generate ASF License warnings. |
| | | 263m 22s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/920 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux fe9ca6f1804b 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | master / fe7c46c |
| Default Java | Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/2/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/2/artifact/yetus-general-check/output/patch-unit-phoenix-core.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/2/testReport/ |
| Max. process+thread count | 6070 (vs. ulimit of 30000) |
| modules | C: phoenix-core U: phoenix-core |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/2/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #920: PHOENIX-6129 : Optimize tableExists() call while retrieving correct MUTEX table
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #920:
URL: https://github.com/apache/phoenix/pull/920#issuecomment-713905498
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 36s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 15m 51s | master passed |
| +1 :green_heart: | compile | 1m 11s | master passed |
| +1 :green_heart: | checkstyle | 1m 17s | master passed |
| +1 :green_heart: | javadoc | 0m 55s | master passed |
| +0 :ok: | spotbugs | 3m 35s | phoenix-core in master has 970 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 9m 53s | the patch passed |
| +1 :green_heart: | compile | 1m 6s | the patch passed |
| +1 :green_heart: | javac | 1m 6s | the patch passed |
| -1 :x: | checkstyle | 1m 18s | phoenix-core: The patch generated 32 new + 2194 unchanged - 50 fixed = 2226 total (was 2244) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 53s | the patch passed |
| +1 :green_heart: | spotbugs | 4m 17s | phoenix-core generated 0 new + 969 unchanged - 1 fixed = 969 total (was 970) |
||| _ Other Tests _ |
| -1 :x: | unit | 111m 28s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 30s | The patch does not generate ASF License warnings. |
| | | 155m 33s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | phoenix.end2end.TableSnapshotReadsMapReduceIT |
| | phoenix.end2end.index.LocalImmutableNonTxIndexIT |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/7/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/920 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux 7845ebd69e68 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | master / 8c81131 |
| Default Java | Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/7/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/7/artifact/yetus-general-check/output/patch-unit-phoenix-core.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/7/testReport/ |
| Max. process+thread count | 6514 (vs. ulimit of 30000) |
| modules | C: phoenix-core U: phoenix-core |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/7/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani commented on pull request #920: PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #920:
URL: https://github.com/apache/phoenix/pull/920#issuecomment-708372414
Since it is not allowed to mutate SYSTEM tables, providing corresponding IT test seems difficult.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ChinmaySKulkarni merged pull request #920: PHOENIX-6129 : Optimize tableExists() call while retrieving correct MUTEX table
Posted by GitBox <gi...@apache.org>.
ChinmaySKulkarni merged pull request #920:
URL: https://github.com/apache/phoenix/pull/920
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani commented on pull request #920: PHOENIX-6129 : Optimize tableExists() call while retrieving correct MUTEX table
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #920:
URL: https://github.com/apache/phoenix/pull/920#issuecomment-713828158
@ChinmaySKulkarni I have addressed your comments, updated Jira message and created Jira [PHOENIX-6203](https://issues.apache.org/jira/browse/PHOENIX-6203) for TNFE issue.
Could you please take a look?
Thanks for the review!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani commented on a change in pull request #920: PHOENIX-6129 : Optimize tableExists() call while retrieving correct MUTEX table
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r509509727
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4364,17 +4353,17 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName
}
}
- private byte[] getSysMutexPhysicalTableNameBytes() throws IOException, SQLException {
- byte[] sysMutexPhysicalTableNameBytes = null;
- try(Admin admin = getAdmin()) {
- if(admin.tableExists(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME)) {
- sysMutexPhysicalTableNameBytes = PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME_BYTES;
- } else if (admin.tableExists(TableName.valueOf(
- SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName()))) {
- sysMutexPhysicalTableNameBytes = SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName();
+ private Table getSysMutexTable() throws SQLException, IOException {
+ String table = SYSTEM_MUTEX_NAME;
+ TableName tableName = TableName.valueOf(table);
+ try (Admin admin = getAdmin()) {
+ if (!admin.tableExists(tableName)) {
+ table = table.replace(QueryConstants.NAME_SEPARATOR,
+ QueryConstants.NAMESPACE_SEPARATOR);
+ tableName = TableName.valueOf(table);
}
+ return connection.getTable(tableName);
Review comment:
> This scenario is the same as returning null from the previous getSysMutexPhysicalTableNameBytes() method and then attempting to retrieve a null table.
Basically, we are certain that either `SYSTEM.MUTEX` or `SYSTEM:MUTEX` exist at any point in time right? e.g if we are performing namespace upgrade (e.g MigrateSystemTablesToSystemNamespaceIT test), when SYSTEM.MUTEX no longer exists, if we call `HBaseFactoryProvider.getHTableFactory().getTable()`, it does return `Table` object, it doesn't return null as such. However, using that table object, we can't perform any operation because actual table would not exist.
As for this comment, the scenario is not same as previous `getSysMutexPhysicalTableNameBytes()` because when we reach at this point, based on above `Admin.tableExists()` call, we would call `connection.getTable()` with correct table name only (I hope we never have situation where SYSTEM.MUTEX and SYSTEM:MUTEX both are dropped. Should we really worry about this case?)
On high level, `connection.getTable()` will never return `null`, it will return new `Table` object even for non existing tables, so we won't encounter NPE due to this for sure. However, I believe we should make just one API call `admin.tableExists()` so that we will know either `SYSTEM.MUTEX or `SYSTEM:MUTEX` exist at any time. Sounds good?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani commented on a change in pull request #920: PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r506072933
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4294,11 +4295,9 @@ public boolean acquireUpgradeMutex(long currentServerSideTableTimestamp)
public boolean writeMutexCell(String tenantId, String schemaName, String tableName,
String columnName, String familyName) throws SQLException {
try {
- byte[] rowKey =
- columnName != null
- ? SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName,
- familyName)
- : SchemaUtil.getTableKey(tenantId, schemaName, tableName);
+ byte[] rowKey = columnName != null ?
+ SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, familyName) :
+ SchemaUtil.getTableKey(tenantId, schemaName, tableName);
// at this point the system mutex table should have been created or
// an exception thrown
byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes();
Review comment:
My bad for this misunderstanding. Addressed concerns, updated the PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] richardantal commented on a change in pull request #920: PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table
Posted by GitBox <gi...@apache.org>.
richardantal commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r504750907
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4294,11 +4295,9 @@ public boolean acquireUpgradeMutex(long currentServerSideTableTimestamp)
public boolean writeMutexCell(String tenantId, String schemaName, String tableName,
String columnName, String familyName) throws SQLException {
try {
- byte[] rowKey =
- columnName != null
- ? SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName,
- familyName)
- : SchemaUtil.getTableKey(tenantId, schemaName, tableName);
+ byte[] rowKey = columnName != null ?
+ SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, familyName) :
+ SchemaUtil.getTableKey(tenantId, schemaName, tableName);
// at this point the system mutex table should have been created or
// an exception thrown
byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes();
Review comment:
`byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes();`
If I understand correctly what @ChinmaySKulkarni described in the ticket, this call will still result to an admin.tableExists call to check the existance of SYSTEM.MUTEX/SYSTEM:MUTEX and you didn't changed that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ChinmaySKulkarni commented on a change in pull request #920: PHOENIX-6129 : Optimize tableExists() call while retrieving correct MUTEX table
Posted by GitBox <gi...@apache.org>.
ChinmaySKulkarni commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r509482018
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4364,17 +4353,17 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName
}
}
- private byte[] getSysMutexPhysicalTableNameBytes() throws IOException, SQLException {
- byte[] sysMutexPhysicalTableNameBytes = null;
- try(Admin admin = getAdmin()) {
- if(admin.tableExists(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME)) {
- sysMutexPhysicalTableNameBytes = PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME_BYTES;
- } else if (admin.tableExists(TableName.valueOf(
- SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName()))) {
- sysMutexPhysicalTableNameBytes = SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName();
+ private Table getSysMutexTable() throws SQLException, IOException {
+ String table = SYSTEM_MUTEX_NAME;
+ TableName tableName = TableName.valueOf(table);
+ try (Admin admin = getAdmin()) {
+ if (!admin.tableExists(tableName)) {
+ table = table.replace(QueryConstants.NAME_SEPARATOR,
+ QueryConstants.NAMESPACE_SEPARATOR);
+ tableName = TableName.valueOf(table);
}
+ return connection.getTable(tableName);
Review comment:
Just want to clarify one thing based on your comment about `connection.getTable()`. At this point, SYS.MUTEX doesn't exist so we try to retrieve SYS:MUTEX. As per your findings, connection.getTable(SYS:MUTEX) would return a `Table` object however it is not guaranteed that the table actually exists. This scenario is the same as returning null from the previous `getSysMutexPhysicalTableNameBytes()` method and then attempting to retrieve a `null` table.
Can you confirm that this scenario (probably an impossible one albeit) is safe and we don't run into any weirdness?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4364,17 +4353,17 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName
}
}
- private byte[] getSysMutexPhysicalTableNameBytes() throws IOException, SQLException {
- byte[] sysMutexPhysicalTableNameBytes = null;
- try(Admin admin = getAdmin()) {
- if(admin.tableExists(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME)) {
- sysMutexPhysicalTableNameBytes = PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME_BYTES;
- } else if (admin.tableExists(TableName.valueOf(
- SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName()))) {
- sysMutexPhysicalTableNameBytes = SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName();
+ private Table getSysMutexTable() throws SQLException, IOException {
+ String table = SYSTEM_MUTEX_NAME;
Review comment:
nit: `table` variable is unnecessary here
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4364,17 +4353,17 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName
}
}
- private byte[] getSysMutexPhysicalTableNameBytes() throws IOException, SQLException {
- byte[] sysMutexPhysicalTableNameBytes = null;
- try(Admin admin = getAdmin()) {
- if(admin.tableExists(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME)) {
- sysMutexPhysicalTableNameBytes = PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME_BYTES;
- } else if (admin.tableExists(TableName.valueOf(
- SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName()))) {
- sysMutexPhysicalTableNameBytes = SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName();
+ private Table getSysMutexTable() throws SQLException, IOException {
Review comment:
Please add a unit test for this new method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ChinmaySKulkarni commented on a change in pull request #920: PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table
Posted by GitBox <gi...@apache.org>.
ChinmaySKulkarni commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r505900118
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4294,11 +4295,9 @@ public boolean acquireUpgradeMutex(long currentServerSideTableTimestamp)
public boolean writeMutexCell(String tenantId, String schemaName, String tableName,
String columnName, String familyName) throws SQLException {
try {
- byte[] rowKey =
- columnName != null
- ? SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName,
- familyName)
- : SchemaUtil.getTableKey(tenantId, schemaName, tableName);
+ byte[] rowKey = columnName != null ?
+ SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, familyName) :
+ SchemaUtil.getTableKey(tenantId, schemaName, tableName);
// at this point the system mutex table should have been created or
// an exception thrown
byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes();
Review comment:
@virajjasani I think there is some confusion here. The aim of this Jira is to reduce the HBase calls to get the table. Currently, the call to `writeMutexCell()` calls `getSysMutexPhysicalTableNameBytes()` which does 1 or 2 HBase admin calls (tableExists()) and then we still do a `getTable()` call [here](https://github.com/apache/phoenix/blob/628fa0db239985b2972f51459540f7513d182885/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java#L4305).
The same happens for `deleteMutexCell()`.
Instead of calling `getSysMutexPhysicalTableNameBytes()`, we can do 1 `getTable()` call with `SYSTEM.MUTEX` and if that throws a TNFE, try again with `SYSTEM:MUTEX` thus eliminating the `tableExists()` calls.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani commented on a change in pull request #920: PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r504760069
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4294,11 +4295,9 @@ public boolean acquireUpgradeMutex(long currentServerSideTableTimestamp)
public boolean writeMutexCell(String tenantId, String schemaName, String tableName,
String columnName, String familyName) throws SQLException {
try {
- byte[] rowKey =
- columnName != null
- ? SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName,
- familyName)
- : SchemaUtil.getTableKey(tenantId, schemaName, tableName);
+ byte[] rowKey = columnName != null ?
+ SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, familyName) :
+ SchemaUtil.getTableKey(tenantId, schemaName, tableName);
// at this point the system mutex table should have been created or
// an exception thrown
byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes();
Review comment:
Here is the throw part: https://github.com/apache/phoenix/pull/920/files#diff-539e6818644fa837ebee4e4b61f4c570f75ac439437050c9a0459762ef8e385cR4373
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani commented on a change in pull request #920: PHOENIX-6129 : Optimize tableExists() call while retrieving correct MUTEX table
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r509509727
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4364,17 +4353,17 @@ public void deleteMutexCell(String tenantId, String schemaName, String tableName
}
}
- private byte[] getSysMutexPhysicalTableNameBytes() throws IOException, SQLException {
- byte[] sysMutexPhysicalTableNameBytes = null;
- try(Admin admin = getAdmin()) {
- if(admin.tableExists(PhoenixDatabaseMetaData.SYSTEM_MUTEX_HBASE_TABLE_NAME)) {
- sysMutexPhysicalTableNameBytes = PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME_BYTES;
- } else if (admin.tableExists(TableName.valueOf(
- SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName()))) {
- sysMutexPhysicalTableNameBytes = SchemaUtil.getPhysicalTableName(SYSTEM_MUTEX_NAME, props).getName();
+ private Table getSysMutexTable() throws SQLException, IOException {
+ String table = SYSTEM_MUTEX_NAME;
+ TableName tableName = TableName.valueOf(table);
+ try (Admin admin = getAdmin()) {
+ if (!admin.tableExists(tableName)) {
+ table = table.replace(QueryConstants.NAME_SEPARATOR,
+ QueryConstants.NAMESPACE_SEPARATOR);
+ tableName = TableName.valueOf(table);
}
+ return connection.getTable(tableName);
Review comment:
> This scenario is the same as returning null from the previous getSysMutexPhysicalTableNameBytes() method and then attempting to retrieve a null table.
Basically, we are certain that either `SYSTEM.MUTEX` or `SYSTEM:MUTEX` exist at any point in time right? e.g if we are performing namespace upgrade (e.g MigrateSystemTablesToSystemNamespaceIT test), when SYSTEM.MUTEX no longer exists, if we call `HBaseFactoryProvider.getHTableFactory().getTable()`, it does return `Table` object, it doesn't return null as such. However, using that table object, we can't perform any operation because actual table would not exist.
As for this comment, the scenario is not same as previous `getSysMutexPhysicalTableNameBytes()` because when we reach at this point, based on above `Admin.tableExists()` call, we would call `connection.getTable()` with correct table name only (I hope we never have situation where SYSTEM.MUTEX and SYSTEM:MUTEX both are dropped. Should we really worry about this case?)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ChinmaySKulkarni commented on pull request #920: PHOENIX-6129 : Optimize tableExists() call while retrieving correct MUTEX table
Posted by GitBox <gi...@apache.org>.
ChinmaySKulkarni commented on pull request #920:
URL: https://github.com/apache/phoenix/pull/920#issuecomment-713742801
@virajjasani please also make the commit message the same as the Jira (or update the Jira if you think this is more appropriate).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani commented on pull request #920: PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #920:
URL: https://github.com/apache/phoenix/pull/920#issuecomment-709339348
Could you please take a look @yanxinyi ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #920: PHOENIX-6129 : Use same connection for tableExists() and getTable() in same method to retrieve correct MUTEX table
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #920:
URL: https://github.com/apache/phoenix/pull/920#issuecomment-710669614
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 59s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| -1 :x: | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
||| _ master Compile Tests _ |
| -1 :x: | mvninstall | 44m 54s | root in master failed. |
| +1 :green_heart: | compile | 1m 1s | master passed |
| +1 :green_heart: | checkstyle | 1m 7s | master passed |
| +1 :green_heart: | javadoc | 0m 47s | master passed |
| +0 :ok: | spotbugs | 3m 9s | phoenix-core in master has 973 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| -1 :x: | mvninstall | 41m 48s | root in the patch failed. |
| +1 :green_heart: | compile | 0m 55s | the patch passed |
| +1 :green_heart: | javac | 0m 55s | the patch passed |
| -1 :x: | checkstyle | 1m 7s | phoenix-core: The patch generated 2 new + 2221 unchanged - 21 fixed = 2223 total (was 2242) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 46s | the patch passed |
| +1 :green_heart: | spotbugs | 3m 16s | phoenix-core generated 0 new + 972 unchanged - 1 fixed = 972 total (was 973) |
||| _ Other Tests _ |
| -1 :x: | unit | 154m 15s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 21s | The patch does not generate ASF License warnings. |
| | | 256m 51s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | phoenix.end2end.OrphanViewToolIT |
| | phoenix.end2end.index.ViewIndexIT |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/5/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/920 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux 8d5b1a70eb70 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | master / 628fa0d |
| Default Java | Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08 |
| mvninstall | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/5/artifact/yetus-general-check/output/branch-mvninstall-root.txt |
| mvninstall | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/5/artifact/yetus-general-check/output/patch-mvninstall-root.txt |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/5/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/5/artifact/yetus-general-check/output/patch-unit-phoenix-core.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/5/testReport/ |
| Max. process+thread count | 6183 (vs. ulimit of 30000) |
| modules | C: phoenix-core U: phoenix-core |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-920/5/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] richardantal commented on a change in pull request #920: PHOENIX-6129 : Avoid redundant Admin API call in the absence of SYSTEM.MUTEX table
Posted by GitBox <gi...@apache.org>.
richardantal commented on a change in pull request #920:
URL: https://github.com/apache/phoenix/pull/920#discussion_r504750907
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4294,11 +4295,9 @@ public boolean acquireUpgradeMutex(long currentServerSideTableTimestamp)
public boolean writeMutexCell(String tenantId, String schemaName, String tableName,
String columnName, String familyName) throws SQLException {
try {
- byte[] rowKey =
- columnName != null
- ? SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName,
- familyName)
- : SchemaUtil.getTableKey(tenantId, schemaName, tableName);
+ byte[] rowKey = columnName != null ?
+ SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, familyName) :
+ SchemaUtil.getTableKey(tenantId, schemaName, tableName);
// at this point the system mutex table should have been created or
// an exception thrown
byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes();
Review comment:
If I understand correctly what @ChinmaySKulkarni described in the ticket, this call will still result to an admin.tableExists call to check the existance of SYSTEM.MUTEX/SYSTEM:MUTEX and you didn't changed that.
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4294,11 +4295,9 @@ public boolean acquireUpgradeMutex(long currentServerSideTableTimestamp)
public boolean writeMutexCell(String tenantId, String schemaName, String tableName,
String columnName, String familyName) throws SQLException {
try {
- byte[] rowKey =
- columnName != null
- ? SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName,
- familyName)
- : SchemaUtil.getTableKey(tenantId, schemaName, tableName);
+ byte[] rowKey = columnName != null ?
+ SchemaUtil.getColumnKey(tenantId, schemaName, tableName, columnName, familyName) :
+ SchemaUtil.getTableKey(tenantId, schemaName, tableName);
// at this point the system mutex table should have been created or
// an exception thrown
byte[] sysMutexPhysicalTableNameBytes = getSysMutexPhysicalTableNameBytes();
Review comment:
Instead We could try the Table sysMutexTable =getTable() call with one of them and catch HBase TableNotFoundException, in that case try with the other one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] virajjasani commented on pull request #920: PHOENIX-6129 : Optimize tableExists() call while retrieving correct MUTEX table
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #920:
URL: https://github.com/apache/phoenix/pull/920#issuecomment-715388193
@ChinmaySKulkarni please take a look when you get time. Failed tests are not relevant.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org