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/11/10 16:31:18 UTC
[GitHub] [phoenix] virajjasani opened a new pull request #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
virajjasani opened a new pull request #960:
URL: https://github.com/apache/phoenix/pull/960
----------------------------------------------------------------
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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
virajjasani edited a comment on pull request #960:
URL: https://github.com/apache/phoenix/pull/960#issuecomment-727226049
> All of the new fields should be added as `optional` ones
The only new proto field added apart from new `TaskMetaDataService` service is a new enum value in `MutationCode` defined in `MetaDataService.proto`. Since it's conversion to `MetaDataProtocol.MutationCode` is done using array ordinal (`result.returnCode = MutationCode.values()[proto.getReturnCode().ordinal()];`), we should be good here because only after system tables are upgraded and client is upgraded to 4.16, new coprocessor will be loaded and used and only after that if we get new enum value of `MutationCode` as callback result of invoking coprocessor, conversion to new enum value of `MetaDataProtocol.MutationCode` will take place at client side. Until client is upgraded to 4.16, new enum value won't be converted using ordinal based formula, which is safe for backward compatibility IMHO.
Please correct me if I am wrong.
----------------------------------------------------------------
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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #960:
URL: https://github.com/apache/phoenix/pull/960#issuecomment-728899466
Just added one more backward compatibility test.
----------------------------------------------------------------
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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #960:
URL: https://github.com/apache/phoenix/pull/960#issuecomment-729162186
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 10s | 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 _ |
| +0 :ok: | mvndep | 3m 23s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 11m 34s | master passed |
| +1 :green_heart: | compile | 1m 26s | master passed |
| +1 :green_heart: | checkstyle | 5m 3s | master passed |
| +1 :green_heart: | javadoc | 1m 58s | master passed |
| +0 :ok: | spotbugs | 3m 9s | phoenix-core in master has 962 extant spotbugs warnings. |
| +0 :ok: | spotbugs | 4m 12s | root in master has 1011 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 10s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 9m 15s | the patch passed |
| +1 :green_heart: | compile | 1m 24s | the patch passed |
| +1 :green_heart: | cc | 1m 24s | the patch passed |
| +1 :green_heart: | javac | 1m 24s | the patch passed |
| -1 :x: | checkstyle | 5m 6s | root: The patch generated 598 new + 16101 unchanged - 130 fixed = 16699 total (was 16231) |
| +1 :green_heart: | prototool | 0m 3s | There were no new prototool issues. |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 1m 52s | the patch passed |
| -1 :x: | spotbugs | 3m 22s | phoenix-core generated 3 new + 962 unchanged - 0 fixed = 965 total (was 962) |
| -1 :x: | spotbugs | 4m 24s | root generated 3 new + 1011 unchanged - 0 fixed = 1014 total (was 1011) |
||| _ Other Tests _ |
| -1 :x: | unit | 153m 55s | root in the patch failed. |
| +1 :green_heart: | asflicense | 0m 45s | The patch does not generate ASF License warnings. |
| | | 215m 16s | |
| Reason | Tests |
|-------:|:------|
| FindBugs | module:phoenix-core |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| FindBugs | module:root |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| 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-960/8/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/960 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile cc prototool |
| uname | Linux 561870db5457 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 / 243ac64 |
| 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-960/8/artifact/yetus-general-check/output/diff-checkstyle-root.txt |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/8/artifact/yetus-general-check/output/new-spotbugs-phoenix-core.html |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/8/artifact/yetus-general-check/output/new-spotbugs-root.html |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/8/artifact/yetus-general-check/output/patch-unit-root.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/8/testReport/ |
| Max. process+thread count | 6302 (vs. ulimit of 30000) |
| modules | C: phoenix-core . U: . |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/8/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 prototool=1.10.0-dev |
| 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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
ChinmaySKulkarni commented on a change in pull request #960:
URL: https://github.com/apache/phoenix/pull/960#discussion_r520956276
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/exception/SQLExceptionCode.java
##########
@@ -431,6 +431,8 @@ public SQLException newException(SQLExceptionInfo info) {
PTable.LinkType.CHILD_TABLE + ") for view"),
TABLE_NOT_IN_REGION(1145, "XCL45", "No modifications allowed on this table. "
+ "Table not in this region."),
+ UNABLE_TO_UPSERT_TASK(1146, "XCL46",
+ "Error upserting records in Task system table"),
Review comment:
nit: Change to "SYSTEM.TASK" table
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/task/Task.java
##########
@@ -75,6 +85,38 @@ public Void run() throws Exception {
}
}
+ private static List<Mutation> getMutationsForSystemTaskTable(
+ PhoenixConnection conn, PreparedStatement stmt,
+ boolean accessCheckEnabled) throws IOException {
+ // we need to mutate SYSTEM.TASK with HBase/login user if access is enabled.
+ if (accessCheckEnabled) {
+ return User.runAsLoginUser(() -> {
+ final RpcCall rpcContext = RpcUtil.getRpcContext();
+ // setting RPC context as null so that user can be reset
+ try {
+ RpcUtil.setRpcContext(null);
+ stmt.execute();
+ // retrieve mutations for SYSTEM.TASK upsert query
+ return conn.getMutationState().toMutations().next()
Review comment:
If other mutations were made using the same connection, we will be returning the joined state of all of them here right? Don't we want to restrict to just returning mutations corresponding to the upsert into the SYSTEM.TASK table?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -1094,6 +1095,11 @@ private void addCoprocessors(byte[] tableName, TableDescriptorBuilder builder,
if(!newDesc.hasCoprocessor(TaskRegionObserver.class.getName())) {
builder.addCoprocessor(TaskRegionObserver.class.getName(), null, priority, null);
}
+ if (!newDesc.hasCoprocessor(
Review comment:
If SYSTEM.TASK already exists on a cluster and you upgrade to 4.16 server bits and connect with a 4.16 client, will we still install this new coproc or does that need extra steps in the upgrade path?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/task/Task.java
##########
@@ -141,12 +181,67 @@ public static void addTask(PhoenixConnection conn, PTable.TaskType taskType, Str
PhoenixDatabaseMetaData.TASK_END_TS + ", " +
PhoenixDatabaseMetaData.TASK_DATA +
" ) VALUES(?,?,?,?,?,?,?,?,?)");
- stmt = setValuesToAddTaskPS(stmt, taskType, tenantId, schemaName, tableName, taskStatus, data, priority, startTs, endTs);
- LOGGER.info("Adding task " + taskType + "," +tableName + "," + taskStatus + "," + startTs, ","+endTs);
+ stmt = setValuesToAddTaskPS(stmt, systemTaskParams.getTaskType(),
+ systemTaskParams.getTenantId(),
+ systemTaskParams.getSchemaName(),
+ systemTaskParams.getTableName(),
+ systemTaskParams.getTaskStatus(), systemTaskParams.getData(),
+ systemTaskParams.getPriority(), systemTaskParams.getStartTs(),
+ systemTaskParams.getEndTs());
+ LOGGER.info("Adding task type: {} , tableName: {} , taskStatus: {}"
+ + " , startTs: {} , endTs: {}", systemTaskParams.getTaskType(),
+ systemTaskParams.getTableName(),
+ systemTaskParams.getTaskStatus(), systemTaskParams.getStartTs(),
+ systemTaskParams.getEndTs());
} catch (SQLException e) {
throw new IOException(e);
}
- mutateSystemTaskTable(conn, stmt, accessCheckEnabled);
+ // if query is getting executed by client (useTaskEndpoint is false),
Review comment:
I'm not sure I understand this comment. We want to use the endpoint if triggered from client-side right? Can you please explain the use of `useTaskEndpoint`?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java
##########
@@ -4668,11 +4669,32 @@ public MutationState alterIndex(AlterIndexStatement statement) throws SQLExcepti
}};
try {
String json = JacksonUtil.getObjectWriter().writeValueAsString(props);
- Task.addTask(connection, PTable.TaskType.INDEX_REBUILD,
- tenantId, schemaName,
- dataTableName, PTable.TaskStatus.CREATED.toString(),
- json, null, ts, null, true);
- connection.commit();
+ List<Mutation> sysTaskUpsertMutations = Task.getMutationsForAddTask(new SystemTaskParams.SystemTaskParamsBuilder()
+ .setConn(connection)
+ .setTaskType(
+ PTable.TaskType.INDEX_REBUILD)
+ .setTenantId(tenantId)
+ .setSchemaName(schemaName)
+ .setTableName(dataTableName)
+ .setTaskStatus(
+ PTable.TaskStatus.CREATED.toString())
+ .setData(json)
+ .setPriority(null)
+ .setStartTs(ts)
+ .setEndTs(null)
+ .setAccessCheckEnabled(true)
+ .setUseTaskEndpoint(true).build());
Review comment:
I'm unclear on the use of `useTaskEndpoint`. Here we are calling the endpoint anyways. In which cases are we planning on not using the endpoint? server-side invocations?
----------------------------------------------------------------
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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #960:
URL: https://github.com/apache/phoenix/pull/960#issuecomment-729033253
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 8s | 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 _ |
| +0 :ok: | mvndep | 3m 12s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 11m 29s | master passed |
| +1 :green_heart: | compile | 1m 26s | master passed |
| +1 :green_heart: | checkstyle | 5m 3s | master passed |
| +1 :green_heart: | javadoc | 2m 0s | master passed |
| +0 :ok: | spotbugs | 3m 7s | phoenix-core in master has 962 extant spotbugs warnings. |
| +0 :ok: | spotbugs | 4m 9s | root in master has 1011 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 10s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 9m 21s | the patch passed |
| +1 :green_heart: | compile | 1m 26s | the patch passed |
| +1 :green_heart: | cc | 1m 26s | the patch passed |
| +1 :green_heart: | javac | 1m 26s | the patch passed |
| -1 :x: | checkstyle | 5m 13s | root: The patch generated 527 new + 16172 unchanged - 59 fixed = 16699 total (was 16231) |
| +1 :green_heart: | prototool | 0m 1s | There were no new prototool issues. |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 1m 55s | the patch passed |
| -1 :x: | spotbugs | 3m 24s | phoenix-core generated 3 new + 962 unchanged - 0 fixed = 965 total (was 962) |
| -1 :x: | spotbugs | 4m 27s | root generated 3 new + 1011 unchanged - 0 fixed = 1014 total (was 1011) |
||| _ Other Tests _ |
| -1 :x: | unit | 152m 47s | root in the patch failed. |
| +1 :green_heart: | asflicense | 0m 44s | The patch does not generate ASF License warnings. |
| | | 214m 5s | |
| Reason | Tests |
|-------:|:------|
| FindBugs | module:phoenix-core |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| FindBugs | module:root |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| 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-960/7/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/960 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile cc prototool |
| uname | Linux b0c52e0ff2d1 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 / 243ac64 |
| 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-960/7/artifact/yetus-general-check/output/diff-checkstyle-root.txt |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/7/artifact/yetus-general-check/output/new-spotbugs-phoenix-core.html |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/7/artifact/yetus-general-check/output/new-spotbugs-root.html |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/7/artifact/yetus-general-check/output/patch-unit-root.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/7/testReport/ |
| Max. process+thread count | 6181 (vs. ulimit of 30000) |
| modules | C: phoenix-core . U: . |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/7/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 prototool=1.10.0-dev |
| 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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #960:
URL: https://github.com/apache/phoenix/pull/960#issuecomment-728884539
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 58s | 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 _ |
| +0 :ok: | mvndep | 3m 36s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 14m 46s | master passed |
| +1 :green_heart: | compile | 1m 47s | master passed |
| +1 :green_heart: | checkstyle | 6m 19s | master passed |
| +1 :green_heart: | javadoc | 2m 38s | master passed |
| +0 :ok: | spotbugs | 3m 45s | phoenix-core in master has 962 extant spotbugs warnings. |
| +0 :ok: | spotbugs | 5m 9s | root in master has 1011 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 13m 4s | the patch passed |
| +1 :green_heart: | compile | 1m 53s | the patch passed |
| +1 :green_heart: | cc | 1m 53s | the patch passed |
| +1 :green_heart: | javac | 1m 53s | the patch passed |
| -1 :x: | checkstyle | 7m 7s | root: The patch generated 527 new + 16172 unchanged - 59 fixed = 16699 total (was 16231) |
| +1 :green_heart: | prototool | 0m 2s | There were no new prototool issues. |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 2m 50s | the patch passed |
| -1 :x: | spotbugs | 4m 16s | phoenix-core generated 3 new + 962 unchanged - 0 fixed = 965 total (was 962) |
| -1 :x: | spotbugs | 5m 30s | root generated 3 new + 1011 unchanged - 0 fixed = 1014 total (was 1011) |
||| _ Other Tests _ |
| -1 :x: | unit | 203m 34s | root in the patch failed. |
| +1 :green_heart: | asflicense | 0m 48s | The patch does not generate ASF License warnings. |
| | | 282m 53s | |
| Reason | Tests |
|-------:|:------|
| FindBugs | module:phoenix-core |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| FindBugs | module:root |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| Failed junit tests | phoenix.end2end.index.IndexMetadataIT |
| 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-960/5/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/960 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile cc prototool |
| uname | Linux 070d96371816 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 / 243ac64 |
| 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-960/5/artifact/yetus-general-check/output/diff-checkstyle-root.txt |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/5/artifact/yetus-general-check/output/new-spotbugs-phoenix-core.html |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/5/artifact/yetus-general-check/output/new-spotbugs-root.html |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/5/artifact/yetus-general-check/output/patch-unit-root.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/5/testReport/ |
| Max. process+thread count | 6287 (vs. ulimit of 30000) |
| modules | C: phoenix-core . U: . |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/5/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 prototool=1.10.0-dev |
| 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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #960:
URL: https://github.com/apache/phoenix/pull/960#discussion_r521329651
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -1094,6 +1095,11 @@ private void addCoprocessors(byte[] tableName, TableDescriptorBuilder builder,
if(!newDesc.hasCoprocessor(TaskRegionObserver.class.getName())) {
builder.addCoprocessor(TaskRegionObserver.class.getName(), null, priority, null);
}
+ if (!newDesc.hasCoprocessor(
Review comment:
Yes, we would need to take care of upgrade path. Updated PR with changes in `upgradeSystemTask()` function by handling addition of this new coprocessor, test is available in `BackwardCompatibilityIT` for existence of new coprocessor in updated table descriptor.
Thanks for the suggestion @ChinmaySKulkarni
----------------------------------------------------------------
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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #960:
URL: https://github.com/apache/phoenix/pull/960#discussion_r524928082
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/task/Task.java
##########
@@ -75,6 +87,55 @@ public Void run() throws Exception {
}
}
+ private static List<Mutation> getMutationsForSystemTaskTable(
+ PhoenixConnection conn, PreparedStatement stmt,
+ boolean accessCheckEnabled) throws IOException {
+ // we need to mutate SYSTEM.TASK with HBase/login user if access is enabled.
+ if (accessCheckEnabled) {
+ return User.runAsLoginUser(() -> {
+ final RpcCall rpcContext = RpcUtil.getRpcContext();
+ // setting RPC context as null so that user can be reset
+ try {
+ RpcUtil.setRpcContext(null);
+ stmt.execute();
+ // retrieve mutations for SYSTEM.TASK upsert query
+ Iterator<Pair<byte[], List<Mutation>>> iterator =
+ conn.getMutationState().toMutations();
+ List<Mutation> taskMutations = iterator.next().getSecond();
+ // we are expecting conn to be used for single upsert
Review comment:
Added new connection in `getMutationsForAddTask()` to resolve this.
Thanks
----------------------------------------------------------------
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] yanxinyi commented on a change in pull request #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
yanxinyi commented on a change in pull request #960:
URL: https://github.com/apache/phoenix/pull/960#discussion_r524835690
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/BackwardCompatibilityIT.java
##########
@@ -319,6 +321,10 @@ public void testUpdatedSplitPolicyForSysTask() throws Exception {
+ compatibleClientVersion,
tableDescriptor.getRegionSplitPolicyClassName(),
SystemTaskSplitPolicy.class.getName());
+ assertTrue("Coprocessor " + TaskMetaDataEndpoint.class.getName()
+ + " has not been added with compatible client version: "
+ + compatibleClientVersion, tableDescriptor.hasCoprocessor(
+ TaskMetaDataEndpoint.class.getName()));
Review comment:
nice check for the coproc
----------------------------------------------------------------
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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #960:
URL: https://github.com/apache/phoenix/pull/960#issuecomment-729716455
Test failures are not relevant, confirmed by running them locally.
----------------------------------------------------------------
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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #960:
URL: https://github.com/apache/phoenix/pull/960#discussion_r521330199
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java
##########
@@ -4668,11 +4669,32 @@ public MutationState alterIndex(AlterIndexStatement statement) throws SQLExcepti
}};
try {
String json = JacksonUtil.getObjectWriter().writeValueAsString(props);
- Task.addTask(connection, PTable.TaskType.INDEX_REBUILD,
- tenantId, schemaName,
- dataTableName, PTable.TaskStatus.CREATED.toString(),
- json, null, ts, null, true);
- connection.commit();
+ List<Mutation> sysTaskUpsertMutations = Task.getMutationsForAddTask(new SystemTaskParams.SystemTaskParamsBuilder()
+ .setConn(connection)
+ .setTaskType(
+ PTable.TaskType.INDEX_REBUILD)
+ .setTenantId(tenantId)
+ .setSchemaName(schemaName)
+ .setTableName(dataTableName)
+ .setTaskStatus(
+ PTable.TaskStatus.CREATED.toString())
+ .setData(json)
+ .setPriority(null)
+ .setStartTs(ts)
+ .setEndTs(null)
+ .setAccessCheckEnabled(true)
+ .setUseTaskEndpoint(true).build());
Review comment:
Right, we don't need this specifically if client is going to use separate method for retrieving mutation list. Updated the patch 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] stoty commented on pull request #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #960:
URL: https://github.com/apache/phoenix/pull/960#issuecomment-729658376
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 22s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | 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 _ |
| +0 :ok: | mvndep | 3m 17s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 11m 51s | master passed |
| +1 :green_heart: | compile | 1m 26s | master passed |
| +1 :green_heart: | checkstyle | 5m 5s | master passed |
| +1 :green_heart: | javadoc | 2m 1s | master passed |
| +0 :ok: | spotbugs | 3m 9s | phoenix-core in master has 962 extant spotbugs warnings. |
| +0 :ok: | spotbugs | 4m 24s | root in master has 1011 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 9m 36s | the patch passed |
| +1 :green_heart: | compile | 1m 31s | the patch passed |
| +1 :green_heart: | cc | 1m 31s | the patch passed |
| +1 :green_heart: | javac | 1m 31s | the patch passed |
| -1 :x: | checkstyle | 4m 59s | root: The patch generated 527 new + 16177 unchanged - 60 fixed = 16704 total (was 16237) |
| +1 :green_heart: | prototool | 0m 1s | There were no new prototool issues. |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 1m 58s | the patch passed |
| -1 :x: | spotbugs | 3m 35s | phoenix-core generated 3 new + 962 unchanged - 0 fixed = 965 total (was 962) |
| -1 :x: | spotbugs | 4m 44s | root generated 3 new + 1011 unchanged - 0 fixed = 1014 total (was 1011) |
||| _ Other Tests _ |
| -1 :x: | unit | 188m 9s | root in the patch failed. |
| +1 :green_heart: | asflicense | 0m 48s | The patch does not generate ASF License warnings. |
| | | 251m 24s | |
| Reason | Tests |
|-------:|:------|
| FindBugs | module:phoenix-core |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| FindBugs | module:root |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| Failed junit tests | phoenix.end2end.TableSnapshotReadsMapReduceIT |
| | phoenix.end2end.index.IndexUsageIT |
| | phoenix.end2end.index.IndexMetadataIT |
| 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-960/11/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/960 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile cc prototool |
| uname | Linux 5b9553cd6306 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 / d0c3caf |
| 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-960/11/artifact/yetus-general-check/output/diff-checkstyle-root.txt |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/11/artifact/yetus-general-check/output/new-spotbugs-phoenix-core.html |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/11/artifact/yetus-general-check/output/new-spotbugs-root.html |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/11/artifact/yetus-general-check/output/patch-unit-root.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/11/testReport/ |
| Max. process+thread count | 6093 (vs. ulimit of 30000) |
| modules | C: phoenix-core . U: . |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/11/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 prototool=1.10.0-dev |
| 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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #960:
URL: https://github.com/apache/phoenix/pull/960#discussion_r525905652
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/BackwardCompatibilityIT.java
##########
@@ -319,7 +323,26 @@ public void testUpdatedSplitPolicyForSysTask() throws Exception {
+ compatibleClientVersion,
Review comment:
Done. Thanks @ChinmaySKulkarni
----------------------------------------------------------------
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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #960:
URL: https://github.com/apache/phoenix/pull/960#discussion_r524897168
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/task/Task.java
##########
@@ -141,12 +216,68 @@ public static void addTask(PhoenixConnection conn, PTable.TaskType taskType, Str
PhoenixDatabaseMetaData.TASK_END_TS + ", " +
PhoenixDatabaseMetaData.TASK_DATA +
" ) VALUES(?,?,?,?,?,?,?,?,?)");
- stmt = setValuesToAddTaskPS(stmt, taskType, tenantId, schemaName, tableName, taskStatus, data, priority, startTs, endTs);
- LOGGER.info("Adding task " + taskType + "," +tableName + "," + taskStatus + "," + startTs, ","+endTs);
+ stmt = setValuesToAddTaskPS(stmt, systemTaskParams.getTaskType(),
+ systemTaskParams.getTenantId(),
+ systemTaskParams.getSchemaName(),
+ systemTaskParams.getTableName(),
+ systemTaskParams.getTaskStatus(), systemTaskParams.getData(),
+ systemTaskParams.getPriority(), systemTaskParams.getStartTs(),
+ systemTaskParams.getEndTs());
+ LOGGER.info("Adding task type: {} , tableName: {} , taskStatus: {}"
+ + " , startTs: {} , endTs: {}", systemTaskParams.getTaskType(),
+ systemTaskParams.getTableName(),
+ systemTaskParams.getTaskStatus(), systemTaskParams.getStartTs(),
+ systemTaskParams.getEndTs());
} catch (SQLException e) {
throw new IOException(e);
}
- mutateSystemTaskTable(conn, stmt, accessCheckEnabled);
+ // if query is getting executed by client, do not execute and commit
+ // mutations
+ if (isCommitAllowed) {
Review comment:
> This is used just for the index rebuild task that gets triggered from the client-side, right? For the tasks that are triggered from the server (i.e. from `MetaDataEndpointImpl` and `TaskRO`, we can just directly call `Task.addTask()` right?
That's right, both of above are correct. Sure, let me rename it to `shouldCommit`, that one sounds better.
----------------------------------------------------------------
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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #960:
URL: https://github.com/apache/phoenix/pull/960#issuecomment-724939657
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 5m 33s | 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 _ |
| +0 :ok: | mvndep | 3m 12s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 11m 35s | master passed |
| +1 :green_heart: | compile | 1m 23s | master passed |
| +1 :green_heart: | checkstyle | 5m 2s | master passed |
| +1 :green_heart: | javadoc | 1m 55s | master passed |
| +0 :ok: | spotbugs | 3m 7s | phoenix-core in master has 962 extant spotbugs warnings. |
| +0 :ok: | spotbugs | 4m 6s | root in master has 1011 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 10s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 8m 52s | the patch passed |
| +1 :green_heart: | compile | 1m 24s | the patch passed |
| +1 :green_heart: | cc | 1m 24s | the patch passed |
| +1 :green_heart: | javac | 1m 24s | the patch passed |
| -1 :x: | checkstyle | 5m 6s | root: The patch generated 531 new + 16172 unchanged - 59 fixed = 16703 total (was 16231) |
| +1 :green_heart: | prototool | 0m 3s | There were no new prototool issues. |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 1m 53s | the patch passed |
| -1 :x: | spotbugs | 3m 23s | phoenix-core generated 9 new + 962 unchanged - 0 fixed = 971 total (was 962) |
| -1 :x: | spotbugs | 4m 26s | root generated 9 new + 1011 unchanged - 0 fixed = 1020 total (was 1011) |
||| _ Other Tests _ |
| -1 :x: | unit | 155m 8s | root in the patch failed. |
| +1 :green_heart: | asflicense | 0m 45s | The patch does not generate ASF License warnings. |
| | | 220m 1s | |
| Reason | Tests |
|-------:|:------|
| FindBugs | module:phoenix-core |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| | org.apache.phoenix.schema.task.SystemTaskParams.getEndTs() may expose internal representation by returning SystemTaskParams.endTs At SystemTaskParams.java:by returning SystemTaskParams.endTs At SystemTaskParams.java:[line 99] |
| | org.apache.phoenix.schema.task.SystemTaskParams.getStartTs() may expose internal representation by returning SystemTaskParams.startTs At SystemTaskParams.java:by returning SystemTaskParams.startTs At SystemTaskParams.java:[line 95] |
| | new org.apache.phoenix.schema.task.SystemTaskParams(PhoenixConnection, PTable$TaskType, String, String, String, String, String, Integer, Timestamp, Timestamp, boolean, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.endTs At SystemTaskParams.java:String, String, String, Integer, Timestamp, Timestamp, boolean, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.endTs At SystemTaskParams.java:[line 57] |
| | new org.apache.phoenix.schema.task.SystemTaskParams(PhoenixConnection, PTable$TaskType, String, String, String, String, String, Integer, Timestamp, Timestamp, boolean, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.startTs At SystemTaskParams.java:String, String, String, Integer, Timestamp, Timestamp, boolean, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.startTs At SystemTaskParams.java:[line 56] |
| | org.apache.phoenix.schema.task.SystemTaskParams$SystemTaskParamsBuilder.setEndTs(Timestamp) may expose internal representation by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.endTs At SystemTaskParams.java:by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.endTs At SystemTaskParams.java:[line 171] |
| | org.apache.phoenix.schema.task.SystemTaskParams$SystemTaskParamsBuilder.setStartTs(Timestamp) may expose internal representation by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.startTs At SystemTaskParams.java:by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.startTs At SystemTaskParams.java:[line 166] |
| FindBugs | module:root |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| | org.apache.phoenix.schema.task.SystemTaskParams.getEndTs() may expose internal representation by returning SystemTaskParams.endTs At SystemTaskParams.java:by returning SystemTaskParams.endTs At SystemTaskParams.java:[line 99] |
| | org.apache.phoenix.schema.task.SystemTaskParams.getStartTs() may expose internal representation by returning SystemTaskParams.startTs At SystemTaskParams.java:by returning SystemTaskParams.startTs At SystemTaskParams.java:[line 95] |
| | new org.apache.phoenix.schema.task.SystemTaskParams(PhoenixConnection, PTable$TaskType, String, String, String, String, String, Integer, Timestamp, Timestamp, boolean, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.endTs At SystemTaskParams.java:String, String, String, Integer, Timestamp, Timestamp, boolean, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.endTs At SystemTaskParams.java:[line 57] |
| | new org.apache.phoenix.schema.task.SystemTaskParams(PhoenixConnection, PTable$TaskType, String, String, String, String, String, Integer, Timestamp, Timestamp, boolean, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.startTs At SystemTaskParams.java:String, String, String, Integer, Timestamp, Timestamp, boolean, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.startTs At SystemTaskParams.java:[line 56] |
| | org.apache.phoenix.schema.task.SystemTaskParams$SystemTaskParamsBuilder.setEndTs(Timestamp) may expose internal representation by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.endTs At SystemTaskParams.java:by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.endTs At SystemTaskParams.java:[line 171] |
| | org.apache.phoenix.schema.task.SystemTaskParams$SystemTaskParamsBuilder.setStartTs(Timestamp) may expose internal representation by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.startTs At SystemTaskParams.java:by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.startTs At SystemTaskParams.java:[line 166] |
| Failed junit tests | phoenix.end2end.ParameterizedIndexUpgradeToolIT |
| | 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-960/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/960 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile cc prototool |
| uname | Linux 4af925c8e842 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 / 28518ff |
| 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-960/1/artifact/yetus-general-check/output/diff-checkstyle-root.txt |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/1/artifact/yetus-general-check/output/new-spotbugs-phoenix-core.html |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/1/artifact/yetus-general-check/output/new-spotbugs-root.html |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/1/artifact/yetus-general-check/output/patch-unit-root.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/1/testReport/ |
| Max. process+thread count | 6157 (vs. ulimit of 30000) |
| modules | C: phoenix-core . U: . |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/1/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 prototool=1.10.0-dev |
| 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 edited a comment on pull request #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
virajjasani edited a comment on pull request #960:
URL: https://github.com/apache/phoenix/pull/960#issuecomment-729716455
Test failures for latest change are not relevant, confirmed by running them locally. And same is the case with 4.x patch results available on Jira as part of Precommit Jira build.
----------------------------------------------------------------
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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #960:
URL: https://github.com/apache/phoenix/pull/960#issuecomment-725438703
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 26s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | 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 _ |
| +0 :ok: | mvndep | 3m 37s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 11m 58s | master passed |
| +1 :green_heart: | compile | 1m 40s | master passed |
| +1 :green_heart: | checkstyle | 5m 4s | master passed |
| +1 :green_heart: | javadoc | 2m 17s | master passed |
| +0 :ok: | spotbugs | 3m 17s | phoenix-core in master has 962 extant spotbugs warnings. |
| +0 :ok: | spotbugs | 4m 59s | root in master has 1011 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 9m 38s | the patch passed |
| +1 :green_heart: | compile | 1m 38s | the patch passed |
| +1 :green_heart: | cc | 1m 38s | the patch passed |
| +1 :green_heart: | javac | 1m 38s | the patch passed |
| -1 :x: | checkstyle | 5m 11s | root: The patch generated 523 new + 16172 unchanged - 59 fixed = 16695 total (was 16231) |
| +1 :green_heart: | prototool | 0m 3s | There were no new prototool issues. |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 2m 9s | the patch passed |
| -1 :x: | spotbugs | 3m 38s | phoenix-core generated 9 new + 962 unchanged - 0 fixed = 971 total (was 962) |
| -1 :x: | spotbugs | 4m 48s | root generated 9 new + 1011 unchanged - 0 fixed = 1020 total (was 1011) |
||| _ Other Tests _ |
| -1 :x: | unit | 169m 41s | root in the patch failed. |
| +1 :green_heart: | asflicense | 0m 43s | The patch does not generate ASF License warnings. |
| | | 235m 5s | |
| Reason | Tests |
|-------:|:------|
| FindBugs | module:phoenix-core |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| | org.apache.phoenix.schema.task.SystemTaskParams.getEndTs() may expose internal representation by returning SystemTaskParams.endTs At SystemTaskParams.java:by returning SystemTaskParams.endTs At SystemTaskParams.java:[line 99] |
| | org.apache.phoenix.schema.task.SystemTaskParams.getStartTs() may expose internal representation by returning SystemTaskParams.startTs At SystemTaskParams.java:by returning SystemTaskParams.startTs At SystemTaskParams.java:[line 95] |
| | new org.apache.phoenix.schema.task.SystemTaskParams(PhoenixConnection, PTable$TaskType, String, String, String, String, String, Integer, Timestamp, Timestamp, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.endTs At SystemTaskParams.java:String, String, String, Integer, Timestamp, Timestamp, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.endTs At SystemTaskParams.java:[line 58] |
| | new org.apache.phoenix.schema.task.SystemTaskParams(PhoenixConnection, PTable$TaskType, String, String, String, String, String, Integer, Timestamp, Timestamp, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.startTs At SystemTaskParams.java:String, String, String, Integer, Timestamp, Timestamp, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.startTs At SystemTaskParams.java:[line 57] |
| | org.apache.phoenix.schema.task.SystemTaskParams$SystemTaskParamsBuilder.setEndTs(Timestamp) may expose internal representation by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.endTs At SystemTaskParams.java:by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.endTs At SystemTaskParams.java:[line 166] |
| | org.apache.phoenix.schema.task.SystemTaskParams$SystemTaskParamsBuilder.setStartTs(Timestamp) may expose internal representation by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.startTs At SystemTaskParams.java:by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.startTs At SystemTaskParams.java:[line 161] |
| FindBugs | module:root |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| | org.apache.phoenix.schema.task.SystemTaskParams.getEndTs() may expose internal representation by returning SystemTaskParams.endTs At SystemTaskParams.java:by returning SystemTaskParams.endTs At SystemTaskParams.java:[line 99] |
| | org.apache.phoenix.schema.task.SystemTaskParams.getStartTs() may expose internal representation by returning SystemTaskParams.startTs At SystemTaskParams.java:by returning SystemTaskParams.startTs At SystemTaskParams.java:[line 95] |
| | new org.apache.phoenix.schema.task.SystemTaskParams(PhoenixConnection, PTable$TaskType, String, String, String, String, String, Integer, Timestamp, Timestamp, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.endTs At SystemTaskParams.java:String, String, String, Integer, Timestamp, Timestamp, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.endTs At SystemTaskParams.java:[line 58] |
| | new org.apache.phoenix.schema.task.SystemTaskParams(PhoenixConnection, PTable$TaskType, String, String, String, String, String, Integer, Timestamp, Timestamp, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.startTs At SystemTaskParams.java:String, String, String, Integer, Timestamp, Timestamp, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.startTs At SystemTaskParams.java:[line 57] |
| | org.apache.phoenix.schema.task.SystemTaskParams$SystemTaskParamsBuilder.setEndTs(Timestamp) may expose internal representation by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.endTs At SystemTaskParams.java:by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.endTs At SystemTaskParams.java:[line 166] |
| | org.apache.phoenix.schema.task.SystemTaskParams$SystemTaskParamsBuilder.setStartTs(Timestamp) may expose internal representation by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.startTs At SystemTaskParams.java:by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.startTs At SystemTaskParams.java:[line 161] |
| Failed junit tests | phoenix.end2end.index.txn.RollbackIT |
| | phoenix.end2end.UpgradeIT |
| 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-960/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/960 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile cc prototool |
| uname | Linux 6b625be7ba32 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 / 8bb8956 |
| 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-960/2/artifact/yetus-general-check/output/diff-checkstyle-root.txt |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/2/artifact/yetus-general-check/output/new-spotbugs-phoenix-core.html |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/2/artifact/yetus-general-check/output/new-spotbugs-root.html |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/2/artifact/yetus-general-check/output/patch-unit-root.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/2/testReport/ |
| Max. process+thread count | 6225 (vs. ulimit of 30000) |
| modules | C: phoenix-core . U: . |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/2/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 prototool=1.10.0-dev |
| 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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #960:
URL: https://github.com/apache/phoenix/pull/960#issuecomment-726725765
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 31s | 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 _ |
| +0 :ok: | mvndep | 3m 21s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 14m 4s | master passed |
| +1 :green_heart: | compile | 1m 38s | master passed |
| +1 :green_heart: | checkstyle | 5m 32s | master passed |
| +1 :green_heart: | javadoc | 2m 19s | master passed |
| +0 :ok: | spotbugs | 3m 57s | phoenix-core in master has 962 extant spotbugs warnings. |
| +0 :ok: | spotbugs | 4m 57s | root in master has 1011 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 10m 36s | the patch passed |
| +1 :green_heart: | compile | 1m 33s | the patch passed |
| +1 :green_heart: | cc | 1m 33s | the patch passed |
| +1 :green_heart: | javac | 1m 33s | the patch passed |
| -1 :x: | checkstyle | 5m 28s | root: The patch generated 594 new + 16101 unchanged - 130 fixed = 16695 total (was 16231) |
| +1 :green_heart: | prototool | 0m 3s | There were no new prototool issues. |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 2m 13s | the patch passed |
| -1 :x: | spotbugs | 3m 51s | phoenix-core generated 3 new + 962 unchanged - 0 fixed = 965 total (was 962) |
| -1 :x: | spotbugs | 5m 30s | root generated 3 new + 1011 unchanged - 0 fixed = 1014 total (was 1011) |
||| _ Other Tests _ |
| -1 :x: | unit | 163m 23s | root in the patch failed. |
| +1 :green_heart: | asflicense | 0m 44s | The patch does not generate ASF License warnings. |
| | | 234m 6s | |
| Reason | Tests |
|-------:|:------|
| FindBugs | module:phoenix-core |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| FindBugs | module:root |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| Failed junit tests | phoenix.end2end.index.IndexMetadataIT |
| 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-960/4/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/960 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile cc prototool |
| uname | Linux 1b57a8b5ad7a 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 / 6cc9d50 |
| 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-960/4/artifact/yetus-general-check/output/diff-checkstyle-root.txt |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/4/artifact/yetus-general-check/output/new-spotbugs-phoenix-core.html |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/4/artifact/yetus-general-check/output/new-spotbugs-root.html |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/4/artifact/yetus-general-check/output/patch-unit-root.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/4/testReport/ |
| Max. process+thread count | 6185 (vs. ulimit of 30000) |
| modules | C: phoenix-core . U: . |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/4/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 prototool=1.10.0-dev |
| 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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #960:
URL: https://github.com/apache/phoenix/pull/960#issuecomment-725559384
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 5s | 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 _ |
| +0 :ok: | mvndep | 3m 10s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 11m 14s | master passed |
| +1 :green_heart: | compile | 1m 24s | master passed |
| +1 :green_heart: | checkstyle | 4m 55s | master passed |
| +1 :green_heart: | javadoc | 1m 59s | master passed |
| +0 :ok: | spotbugs | 3m 8s | phoenix-core in master has 962 extant spotbugs warnings. |
| +0 :ok: | spotbugs | 4m 8s | root in master has 1011 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 10s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 8m 58s | the patch passed |
| +1 :green_heart: | compile | 1m 23s | the patch passed |
| +1 :green_heart: | cc | 1m 23s | the patch passed |
| +1 :green_heart: | javac | 1m 23s | the patch passed |
| -1 :x: | checkstyle | 5m 15s | root: The patch generated 523 new + 16172 unchanged - 59 fixed = 16695 total (was 16231) |
| +1 :green_heart: | prototool | 0m 3s | There were no new prototool issues. |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 1m 56s | the patch passed |
| -1 :x: | spotbugs | 3m 25s | phoenix-core generated 9 new + 962 unchanged - 0 fixed = 971 total (was 962) |
| -1 :x: | spotbugs | 4m 32s | root generated 9 new + 1011 unchanged - 0 fixed = 1020 total (was 1011) |
||| _ Other Tests _ |
| -1 :x: | unit | 153m 34s | root in the patch failed. |
| +1 :green_heart: | asflicense | 0m 43s | The patch does not generate ASF License warnings. |
| | | 214m 6s | |
| Reason | Tests |
|-------:|:------|
| FindBugs | module:phoenix-core |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| | org.apache.phoenix.schema.task.SystemTaskParams.getEndTs() may expose internal representation by returning SystemTaskParams.endTs At SystemTaskParams.java:by returning SystemTaskParams.endTs At SystemTaskParams.java:[line 99] |
| | org.apache.phoenix.schema.task.SystemTaskParams.getStartTs() may expose internal representation by returning SystemTaskParams.startTs At SystemTaskParams.java:by returning SystemTaskParams.startTs At SystemTaskParams.java:[line 95] |
| | new org.apache.phoenix.schema.task.SystemTaskParams(PhoenixConnection, PTable$TaskType, String, String, String, String, String, Integer, Timestamp, Timestamp, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.endTs At SystemTaskParams.java:String, String, String, Integer, Timestamp, Timestamp, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.endTs At SystemTaskParams.java:[line 58] |
| | new org.apache.phoenix.schema.task.SystemTaskParams(PhoenixConnection, PTable$TaskType, String, String, String, String, String, Integer, Timestamp, Timestamp, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.startTs At SystemTaskParams.java:String, String, String, Integer, Timestamp, Timestamp, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.startTs At SystemTaskParams.java:[line 57] |
| | org.apache.phoenix.schema.task.SystemTaskParams$SystemTaskParamsBuilder.setEndTs(Timestamp) may expose internal representation by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.endTs At SystemTaskParams.java:by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.endTs At SystemTaskParams.java:[line 166] |
| | org.apache.phoenix.schema.task.SystemTaskParams$SystemTaskParamsBuilder.setStartTs(Timestamp) may expose internal representation by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.startTs At SystemTaskParams.java:by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.startTs At SystemTaskParams.java:[line 161] |
| FindBugs | module:root |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| | org.apache.phoenix.schema.task.SystemTaskParams.getEndTs() may expose internal representation by returning SystemTaskParams.endTs At SystemTaskParams.java:by returning SystemTaskParams.endTs At SystemTaskParams.java:[line 99] |
| | org.apache.phoenix.schema.task.SystemTaskParams.getStartTs() may expose internal representation by returning SystemTaskParams.startTs At SystemTaskParams.java:by returning SystemTaskParams.startTs At SystemTaskParams.java:[line 95] |
| | new org.apache.phoenix.schema.task.SystemTaskParams(PhoenixConnection, PTable$TaskType, String, String, String, String, String, Integer, Timestamp, Timestamp, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.endTs At SystemTaskParams.java:String, String, String, Integer, Timestamp, Timestamp, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.endTs At SystemTaskParams.java:[line 58] |
| | new org.apache.phoenix.schema.task.SystemTaskParams(PhoenixConnection, PTable$TaskType, String, String, String, String, String, Integer, Timestamp, Timestamp, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.startTs At SystemTaskParams.java:String, String, String, Integer, Timestamp, Timestamp, boolean) may expose internal representation by storing an externally mutable object into SystemTaskParams.startTs At SystemTaskParams.java:[line 57] |
| | org.apache.phoenix.schema.task.SystemTaskParams$SystemTaskParamsBuilder.setEndTs(Timestamp) may expose internal representation by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.endTs At SystemTaskParams.java:by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.endTs At SystemTaskParams.java:[line 166] |
| | org.apache.phoenix.schema.task.SystemTaskParams$SystemTaskParamsBuilder.setStartTs(Timestamp) may expose internal representation by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.startTs At SystemTaskParams.java:by storing an externally mutable object into SystemTaskParams$SystemTaskParamsBuilder.startTs At SystemTaskParams.java:[line 161] |
| 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-960/3/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/960 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile cc prototool |
| uname | Linux 779f949e7e00 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 / 8bb8956 |
| 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-960/3/artifact/yetus-general-check/output/diff-checkstyle-root.txt |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/3/artifact/yetus-general-check/output/new-spotbugs-phoenix-core.html |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/3/artifact/yetus-general-check/output/new-spotbugs-root.html |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/3/artifact/yetus-general-check/output/patch-unit-root.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/3/testReport/ |
| Max. process+thread count | 6248 (vs. ulimit of 30000) |
| modules | C: phoenix-core . U: . |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/3/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 prototool=1.10.0-dev |
| 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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
ChinmaySKulkarni commented on a change in pull request #960:
URL: https://github.com/apache/phoenix/pull/960#discussion_r524762304
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/task/Task.java
##########
@@ -75,6 +87,55 @@ public Void run() throws Exception {
}
}
+ private static List<Mutation> getMutationsForSystemTaskTable(
+ PhoenixConnection conn, PreparedStatement stmt,
+ boolean accessCheckEnabled) throws IOException {
+ // we need to mutate SYSTEM.TASK with HBase/login user if access is enabled.
+ if (accessCheckEnabled) {
+ return User.runAsLoginUser(() -> {
+ final RpcCall rpcContext = RpcUtil.getRpcContext();
+ // setting RPC context as null so that user can be reset
+ try {
+ RpcUtil.setRpcContext(null);
+ stmt.execute();
+ // retrieve mutations for SYSTEM.TASK upsert query
+ Iterator<Pair<byte[], List<Mutation>>> iterator =
+ conn.getMutationState().toMutations();
+ List<Mutation> taskMutations = iterator.next().getSecond();
+ // we are expecting conn to be used for single upsert
Review comment:
I don't think this is a good idea. Instead why not just create a separate connectionOnServer and ensure that it contains just the mutations related to SYSTEM.TASK?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/task/Task.java
##########
@@ -141,12 +216,68 @@ public static void addTask(PhoenixConnection conn, PTable.TaskType taskType, Str
PhoenixDatabaseMetaData.TASK_END_TS + ", " +
PhoenixDatabaseMetaData.TASK_DATA +
" ) VALUES(?,?,?,?,?,?,?,?,?)");
- stmt = setValuesToAddTaskPS(stmt, taskType, tenantId, schemaName, tableName, taskStatus, data, priority, startTs, endTs);
- LOGGER.info("Adding task " + taskType + "," +tableName + "," + taskStatus + "," + startTs, ","+endTs);
+ stmt = setValuesToAddTaskPS(stmt, systemTaskParams.getTaskType(),
+ systemTaskParams.getTenantId(),
+ systemTaskParams.getSchemaName(),
+ systemTaskParams.getTableName(),
+ systemTaskParams.getTaskStatus(), systemTaskParams.getData(),
+ systemTaskParams.getPriority(), systemTaskParams.getStartTs(),
+ systemTaskParams.getEndTs());
+ LOGGER.info("Adding task type: {} , tableName: {} , taskStatus: {}"
+ + " , startTs: {} , endTs: {}", systemTaskParams.getTaskType(),
+ systemTaskParams.getTableName(),
+ systemTaskParams.getTaskStatus(), systemTaskParams.getStartTs(),
+ systemTaskParams.getEndTs());
} catch (SQLException e) {
throw new IOException(e);
}
- mutateSystemTaskTable(conn, stmt, accessCheckEnabled);
+ // if query is getting executed by client, do not execute and commit
+ // mutations
+ if (isCommitAllowed) {
Review comment:
Can we rename this variable? Sounds like it is set to `false` when we don't really want to commit, but rather just get the stmt object which we later use to get the `List<Mutation>`. This is used just for the index rebuild task that gets triggered from the client-side, right? For the tasks that are triggered from the server (i.e. from `MetaDataEndpointImpl` and `TaskRO`, we can just directly call `Task.addTask()` right?
A more appropriate name would be `shouldCommit` or something.
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/task/Task.java
##########
@@ -75,6 +87,55 @@ public Void run() throws Exception {
}
}
+ private static List<Mutation> getMutationsForSystemTaskTable(
+ PhoenixConnection conn, PreparedStatement stmt,
+ boolean accessCheckEnabled) throws IOException {
+ // we need to mutate SYSTEM.TASK with HBase/login user if access is enabled.
+ if (accessCheckEnabled) {
+ return User.runAsLoginUser(() -> {
+ final RpcCall rpcContext = RpcUtil.getRpcContext();
+ // setting RPC context as null so that user can be reset
+ try {
+ RpcUtil.setRpcContext(null);
+ stmt.execute();
+ // retrieve mutations for SYSTEM.TASK upsert query
+ Iterator<Pair<byte[], List<Mutation>>> iterator =
+ conn.getMutationState().toMutations();
+ List<Mutation> taskMutations = iterator.next().getSecond();
+ // we are expecting conn to be used for single upsert
+ // query on SYSTEM.TASK
+ if (iterator.hasNext()) {
+ throw new IOException("Provided connection should only "
+ + "contain mutations related to SYSTEM.TASK table");
+ }
+ return taskMutations;
+ } catch (SQLException e) {
+ throw new IOException(e);
+ } finally {
+ // setting RPC context back to original context of the RPC
+ RpcUtil.setRpcContext(rpcContext);
+ }
+ });
+ } else {
+ try {
+ stmt.execute();
Review comment:
Can we extract the this part into a small helper method? The `else` case is similar to the `if` just that we aren't running with impersonation
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/task/Task.java
##########
@@ -75,6 +87,55 @@ public Void run() throws Exception {
}
}
+ private static List<Mutation> getMutationsForSystemTaskTable(
+ PhoenixConnection conn, PreparedStatement stmt,
+ boolean accessCheckEnabled) throws IOException {
+ // we need to mutate SYSTEM.TASK with HBase/login user if access is enabled.
+ if (accessCheckEnabled) {
+ return User.runAsLoginUser(() -> {
+ final RpcCall rpcContext = RpcUtil.getRpcContext();
+ // setting RPC context as null so that user can be reset
+ try {
+ RpcUtil.setRpcContext(null);
+ stmt.execute();
+ // retrieve mutations for SYSTEM.TASK upsert query
+ Iterator<Pair<byte[], List<Mutation>>> iterator =
+ conn.getMutationState().toMutations();
+ List<Mutation> taskMutations = iterator.next().getSecond();
+ // we are expecting conn to be used for single upsert
+ // query on SYSTEM.TASK
+ if (iterator.hasNext()) {
Review comment:
Can it not be that the first `next()` gave you mutations corresponding to some other upsert/delete? Let's just use our own connection
----------------------------------------------------------------
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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #960:
URL: https://github.com/apache/phoenix/pull/960#issuecomment-726729877
Spotbugs are related to generate proto class, I hope we can ignore them @ChinmaySKulkarni @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] virajjasani commented on pull request #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #960:
URL: https://github.com/apache/phoenix/pull/960#issuecomment-727226049
> All of the new fields should be added as `optional` ones
The only new proto field added apart from new `TaskMetaDataService` service is a new enum value in `MutationCode` defined in `MetaDataService.proto`. Since it's conversion to `MetaDataProtocol.MutationCode` is done using array ordinal (`result.returnCode = MutationCode.values()[proto.getReturnCode().ordinal()];`), we should be good here because only after system tables are upgraded and client is upgraded to 4.16, new coprocessor will be loaded and used and only after that if we get new enum value of `MutationCode` as callback result of invoking coprocessor, conversion to new enum value of `MetaDataProtocol.MutationCode` will take place at client side.
Please correct me if I am wrong.
----------------------------------------------------------------
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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #960:
URL: https://github.com/apache/phoenix/pull/960#issuecomment-728741716
Thanks @ChinmaySKulkarni for the review, addressed your concerns and added unit tests for `TaskMetaDataEndpoint` also which covers callback class as well.
Let me squash commits.
----------------------------------------------------------------
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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #960:
URL: https://github.com/apache/phoenix/pull/960#issuecomment-729627177
Backward compatibility IT results for 4.x patch with latest changes (all tests passed):
1. https://ci-hadoop.apache.org/job/PreCommit-PHOENIX-Build/195/testReport/org.apache.phoenix.end2end/BackwardCompatibilityIT/
2. https://ci-hadoop.apache.org/job/PreCommit-PHOENIX-Build/196/testReport/org.apache.phoenix.end2end/BackwardCompatibilityIT/
----------------------------------------------------------------
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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #960:
URL: https://github.com/apache/phoenix/pull/960#issuecomment-729870376
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 22s | 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 _ |
| +0 :ok: | mvndep | 3m 22s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 11m 47s | master passed |
| +1 :green_heart: | compile | 1m 26s | master passed |
| +1 :green_heart: | checkstyle | 5m 5s | master passed |
| +1 :green_heart: | javadoc | 2m 3s | master passed |
| +0 :ok: | spotbugs | 3m 11s | phoenix-core in master has 962 extant spotbugs warnings. |
| +0 :ok: | spotbugs | 4m 8s | root in master has 1011 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 10s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 9m 13s | the patch passed |
| +1 :green_heart: | compile | 1m 26s | the patch passed |
| +1 :green_heart: | cc | 1m 26s | the patch passed |
| +1 :green_heart: | javac | 1m 26s | the patch passed |
| -1 :x: | checkstyle | 5m 14s | root: The patch generated 598 new + 16106 unchanged - 131 fixed = 16704 total (was 16237) |
| +1 :green_heart: | prototool | 0m 3s | There were no new prototool issues. |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 1m 55s | the patch passed |
| -1 :x: | spotbugs | 3m 27s | phoenix-core generated 3 new + 962 unchanged - 0 fixed = 965 total (was 962) |
| -1 :x: | spotbugs | 4m 30s | root generated 3 new + 1011 unchanged - 0 fixed = 1014 total (was 1011) |
||| _ Other Tests _ |
| -1 :x: | unit | 174m 46s | root in the patch failed. |
| +1 :green_heart: | asflicense | 0m 44s | The patch does not generate ASF License warnings. |
| | | 237m 6s | |
| Reason | Tests |
|-------:|:------|
| FindBugs | module:phoenix-core |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| FindBugs | module:root |
| | org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest.PARSER isn't final but should be At TaskMetaDataProtos.java:be At TaskMetaDataProtos.java:[line 114] |
| | Class org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest defines non-transient non-serializable instance field unknownFields In TaskMetaDataProtos.java:instance field unknownFields In TaskMetaDataProtos.java |
| | Useless control flow in org.apache.phoenix.coprocessor.generated.TaskMetaDataProtos$TaskMutateRequest$Builder.maybeForceBuilderInitialization() At TaskMetaDataProtos.java: At TaskMetaDataProtos.java:[line 330] |
| 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-960/12/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/960 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile cc prototool |
| uname | Linux 1ada97415aed 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 / d0c3caf |
| 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-960/12/artifact/yetus-general-check/output/diff-checkstyle-root.txt |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/12/artifact/yetus-general-check/output/new-spotbugs-phoenix-core.html |
| spotbugs | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/12/artifact/yetus-general-check/output/new-spotbugs-root.html |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/12/artifact/yetus-general-check/output/patch-unit-root.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/12/testReport/ |
| Max. process+thread count | 6221 (vs. ulimit of 30000) |
| modules | C: phoenix-core . U: . |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-960/12/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 prototool=1.10.0-dev |
| 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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
ChinmaySKulkarni commented on a change in pull request #960:
URL: https://github.com/apache/phoenix/pull/960#discussion_r525555394
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/BackwardCompatibilityIT.java
##########
@@ -319,7 +323,26 @@ public void testUpdatedSplitPolicyForSysTask() throws Exception {
+ compatibleClientVersion,
Review comment:
We should rename this test since we are no longer just checking that the split policy is updated. Also, better if we can do something like the following:
1. Connect with an old client: If <4.15, SYS.TASK doesn't even exist, so nothing to do. If 4.15, confirm that SYS.TASK **doesn't** have the coproc loaded.
2. Then connect with a new client (4.16) which will trigger the metadata upgrade and update the coproc. Now do your check for existence of the coproc. Similarly can be done for the split policy too.
----------------------------------------------------------------
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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
ChinmaySKulkarni merged pull request #960:
URL: https://github.com/apache/phoenix/pull/960
----------------------------------------------------------------
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 #960: PHOENIX-6155 : Provide a coprocessor endpoint to avoid direct upserts into SYSTEM.TASK from the client
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #960:
URL: https://github.com/apache/phoenix/pull/960#discussion_r524928082
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/task/Task.java
##########
@@ -75,6 +87,55 @@ public Void run() throws Exception {
}
}
+ private static List<Mutation> getMutationsForSystemTaskTable(
+ PhoenixConnection conn, PreparedStatement stmt,
+ boolean accessCheckEnabled) throws IOException {
+ // we need to mutate SYSTEM.TASK with HBase/login user if access is enabled.
+ if (accessCheckEnabled) {
+ return User.runAsLoginUser(() -> {
+ final RpcCall rpcContext = RpcUtil.getRpcContext();
+ // setting RPC context as null so that user can be reset
+ try {
+ RpcUtil.setRpcContext(null);
+ stmt.execute();
+ // retrieve mutations for SYSTEM.TASK upsert query
+ Iterator<Pair<byte[], List<Mutation>>> iterator =
+ conn.getMutationState().toMutations();
+ List<Mutation> taskMutations = iterator.next().getSecond();
+ // we are expecting conn to be used for single upsert
Review comment:
Adding new connection to resolve this.
Thanks
----------------------------------------------------------------
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