You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "fjtirado (via GitHub)" <gi...@apache.org> on 2024/03/14 12:00:30 UTC
[PR] [Fix_#3413] Speed up businesskey query performance [incubator-kie-kogito-runtimes]
fjtirado opened a new pull request, #3441:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3441
(no comment)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix_#3413] Speed up businesskey query performance [incubator-kie-kogito-runtimes]
Posted by "fjtirado (via GitHub)" <gi...@apache.org>.
fjtirado commented on code in PR #3441:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3441#discussion_r1526100548
##########
addons/common/persistence/jdbc/src/main/java/org/kie/kogito/persistence/jdbc/GenericRepository.java:
##########
@@ -140,6 +143,19 @@ Optional<Record> findByIdInternal(String processId, String processVersion, UUID
return Optional.empty();
}
+ @Override
+ Optional<Record> findByBusinessKey(String processId, String processVersion, String businessKey) {
+ try (Connection connection = dataSource.getConnection();
+ PreparedStatement statement = connection.prepareStatement(FIND_BY_BUSINESS_KEY)) {
+ statement.setString(1, businessKey);
+ try (ResultSet resultSet = statement.executeQuery()) {
+ return resultSet.next() ? Optional.of(from(resultSet)) : Optional.empty();
+ }
+ } catch (Exception e) {
+ throw uncheckedException(e, "Error finding process instance using business key %s", businessKey);
Review Comment:
yes, thats a good point, it provides more infor for debugging, changing it
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix_#3413] Speed up businesskey query performance [incubator-kie-kogito-runtimes]
Posted by "kie-ci3 (via GitHub)" <gi...@apache.org>.
kie-ci3 commented on PR #3441:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3441#issuecomment-2000044823
**PR job** `#13` was: **UNSTABLE**
Possible explanation: This should be test failures
<details>
<summary><b>Reproducer</b></summary>
build-chain build full_downstream -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-kogito-runtimes -u https://github.com/apache/incubator-kie-kogito-runtimes/pull/3441 --skipParallelCheckout
NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution
</details>
Please look here: https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3441/13/display/redirect
**Test results:**
- PASSED: 3253
- FAILED: 1
Those are the test failures:
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3441/13/testReport/org.kie.kogito.quarkus.workflows/EventFlowIT/testNotStartingEvent/">org.kie.kogito.quarkus.workflows.EventFlowIT.testNotStartingEvent</a></summary>
Condition with alias 'dead' didn't complete within 15 seconds because assertion condition defined as a org.kie.kogito.quarkus.workflows.AssuredTestUtils 1 expectation failed.<br/>Expected status code <404> but was <200>.<br/>.
</details>
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix_#3413] Speed up businesskey query performance [incubator-kie-kogito-runtimes]
Posted by "kie-ci3 (via GitHub)" <gi...@apache.org>.
kie-ci3 commented on PR #3441:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3441#issuecomment-1997676489
**PR job** `#7` was: **UNSTABLE**
Possible explanation: This should be test failures
<details>
<summary><b>Reproducer</b></summary>
build-chain build full_downstream -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-kogito-runtimes -u https://github.com/apache/incubator-kie-kogito-runtimes/pull/3441 --skipParallelCheckout
NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution
</details>
Please look here: https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3441/7/display/redirect
**Test results:**
- PASSED: 3253
- FAILED: 1
Those are the test failures:
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3441/7/testReport/org.kie.kogito.quarkus.workflows/EventFlowIT/testNotStartingEvent/">org.kie.kogito.quarkus.workflows.EventFlowIT.testNotStartingEvent</a></summary>
1 expectation failed.<br/>Expected status code <201> but was <500>.<br/>
</details>
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix_#3413] Speed up businesskey query performance [incubator-kie-kogito-runtimes]
Posted by "wmedvede (via GitHub)" <gi...@apache.org>.
wmedvede commented on code in PR #3441:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3441#discussion_r1526093584
##########
addons/common/persistence/jdbc/src/main/java/org/kie/kogito/persistence/jdbc/GenericRepository.java:
##########
@@ -140,6 +143,19 @@ Optional<Record> findByIdInternal(String processId, String processVersion, UUID
return Optional.empty();
}
+ @Override
+ Optional<Record> findByBusinessKey(String processId, String processVersion, String businessKey) {
+ try (Connection connection = dataSource.getConnection();
+ PreparedStatement statement = connection.prepareStatement(FIND_BY_BUSINESS_KEY)) {
+ statement.setString(1, businessKey);
+ try (ResultSet resultSet = statement.executeQuery()) {
+ return resultSet.next() ? Optional.of(from(resultSet)) : Optional.empty();
+ }
+ } catch (Exception e) {
+ throw uncheckedException(e, "Error finding process instance using business key %s", businessKey);
Review Comment:
I see what you mean, but IMO, in that case, what people is invoking is the interface, and thus, they say, "I want to get a process instance for "my_process", in the version "xxx" and that has the businessKey = "yyyy".
The fact that the interface internally is never using some parameters is a different story.
And thus, If this fails, for me, it makes sense to have message I mentioned, because it helps to link the error to the action you wanted to execute. But np.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix_#3413] Speed up businesskey query performance [incubator-kie-kogito-runtimes]
Posted by "kie-ci3 (via GitHub)" <gi...@apache.org>.
kie-ci3 commented on PR #3441:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3441#issuecomment-1998049223
**PR job** `#8` was: **UNSTABLE**
Possible explanation: This should be test failures
<details>
<summary><b>Reproducer</b></summary>
build-chain build full_downstream -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-kogito-runtimes -u https://github.com/apache/incubator-kie-kogito-runtimes/pull/3441 --skipParallelCheckout
NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution
</details>
Please look here: https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3441/8/display/redirect
**Test results:**
- PASSED: 3253
- FAILED: 1
Those are the test failures:
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3441/8/testReport/org.jbpm.process/TimerTest/testTimer/">org.jbpm.process.TimerTest.testTimer</a></summary>
<br/>expected: 4<br/> but was: 5
</details>
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix_#3413] Speed up businesskey query performance [incubator-kie-kogito-runtimes]
Posted by "wmedvede (via GitHub)" <gi...@apache.org>.
wmedvede commented on code in PR #3441:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3441#discussion_r1525975497
##########
addons/common/persistence/jdbc/src/main/java/org/kie/kogito/persistence/jdbc/GenericRepository.java:
##########
@@ -34,32 +34,35 @@
import javax.sql.DataSource;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
public class GenericRepository extends Repository {
private static final String PAYLOAD = "payload";
private static final String VERSION = "version";
- private static final Logger LOGGER = LoggerFactory.getLogger(GenericRepository.class);
-
private final DataSource dataSource;
public GenericRepository(DataSource dataSource) {
this.dataSource = dataSource;
}
@Override
- void insertInternal(String processId, String processVersion, UUID id, byte[] payload) {
+ void insertInternal(String processId, String processVersion, UUID id, byte[] payload, String businessKey) {
Review Comment:
I believe that analogous to the insertInternal, deleteInternal should remove the associated businessKey if any.
##########
addons/common/persistence/jdbc/src/main/java/org/kie/kogito/persistence/jdbc/GenericRepository.java:
##########
@@ -140,6 +143,19 @@ Optional<Record> findByIdInternal(String processId, String processVersion, UUID
return Optional.empty();
}
+ @Override
+ Optional<Record> findByBusinessKey(String processId, String processVersion, String businessKey) {
+ try (Connection connection = dataSource.getConnection();
+ PreparedStatement statement = connection.prepareStatement(FIND_BY_BUSINESS_KEY)) {
+ statement.setString(1, businessKey);
+ try (ResultSet resultSet = statement.executeQuery()) {
+ return resultSet.next() ? Optional.of(from(resultSet)) : Optional.empty();
+ }
+ } catch (Exception e) {
+ throw uncheckedException(e, "Error finding process instance using business key %s", businessKey);
Review Comment:
Nit pick, but maybe if we add the processId and the processVersion to the exception it can help future error debuging/tracing etc, since we'll know all about the failing query.
##########
addons/common/persistence/jdbc/src/main/java/org/kie/kogito/persistence/jdbc/GenericRepository.java:
##########
@@ -34,32 +34,35 @@
import javax.sql.DataSource;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
public class GenericRepository extends Repository {
private static final String PAYLOAD = "payload";
private static final String VERSION = "version";
- private static final Logger LOGGER = LoggerFactory.getLogger(GenericRepository.class);
-
private final DataSource dataSource;
public GenericRepository(DataSource dataSource) {
this.dataSource = dataSource;
}
@Override
- void insertInternal(String processId, String processVersion, UUID id, byte[] payload) {
+ void insertInternal(String processId, String processVersion, UUID id, byte[] payload, String businessKey) {
try (Connection connection = dataSource.getConnection();
PreparedStatement statement = connection.prepareStatement(INSERT)) {
- statement.setString(1, id.toString());
+ String processInstanceId = id.toString();
+ statement.setString(1, processInstanceId);
statement.setBytes(2, payload);
statement.setString(3, processId);
statement.setString(4, processVersion);
statement.setLong(5, 0L);
statement.executeUpdate();
+ if (businessKey != null) {
+ try (PreparedStatement businessKeyStmt = connection.prepareStatement(INSERT_BUSINESS_KEY)) {
+ businessKeyStmt.setString(1, businessKey);
+ businessKeyStmt.setString(2, processInstanceId);
+ businessKeyStmt.executeUpdate();
+ }
+ }
} catch (Exception e) {
throw uncheckedException(e, "Error inserting process instance %s", id);
Review Comment:
Simillar to the other comment, I believe it could make sense to incorporate the processId, processVersion and businessKey in this message.
##########
addons/common/persistence/jdbc/src/main/resources/db/ansi/V10.0.0__add_business_key_ansi.sql:
##########
@@ -0,0 +1,11 @@
+CREATE TABLE business_key_mapping (
+ name VARCHAR (255) NOT NULL,
Review Comment:
I think it could make sense to name this field business_key instead, wdyt?
##########
addons/common/persistence/jdbc/src/main/resources/db/postgresql/V10.0.0__add_business_key_PostgreSQL.sql:
##########
@@ -0,0 +1,12 @@
+CREATE TABLE business_key_mapping (
+ name character (255) NOT NULL,
Review Comment:
same here?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix_#3413] Speed up businesskey query performance [incubator-kie-kogito-runtimes]
Posted by "fjtirado (via GitHub)" <gi...@apache.org>.
fjtirado commented on code in PR #3441:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3441#discussion_r1526038707
##########
addons/common/persistence/jdbc/src/main/java/org/kie/kogito/persistence/jdbc/GenericRepository.java:
##########
@@ -140,6 +143,19 @@ Optional<Record> findByIdInternal(String processId, String processVersion, UUID
return Optional.empty();
}
+ @Override
+ Optional<Record> findByBusinessKey(String processId, String processVersion, String businessKey) {
+ try (Connection connection = dataSource.getConnection();
+ PreparedStatement statement = connection.prepareStatement(FIND_BY_BUSINESS_KEY)) {
+ statement.setString(1, businessKey);
+ try (ResultSet resultSet = statement.executeQuery()) {
+ return resultSet.next() ? Optional.of(from(resultSet)) : Optional.empty();
+ }
+ } catch (Exception e) {
+ throw uncheckedException(e, "Error finding process instance using business key %s", businessKey);
Review Comment:
Well, here, I leave processId and processVersion in the interface in case in future we change from globally unique (per db) business key to process id unique business key (so we do not have to change the interface, just the schema), but right now, as you can see, these two parameters are unused, so I think logging them might be confusing
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix_#3413] Speed up businesskey query performance [incubator-kie-kogito-runtimes]
Posted by "fjtirado (via GitHub)" <gi...@apache.org>.
fjtirado merged PR #3441:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3441
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix_#3413] Speed up businesskey query performance [incubator-kie-kogito-runtimes]
Posted by "kie-ci3 (via GitHub)" <gi...@apache.org>.
kie-ci3 commented on PR #3441:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3441#issuecomment-1997506245
**PR job** `#6` was: **UNSTABLE**
Possible explanation: This should be test failures
<details>
<summary><b>Reproducer</b></summary>
build-chain build full_downstream -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-kogito-runtimes -u https://github.com/apache/incubator-kie-kogito-runtimes/pull/3441 --skipParallelCheckout
NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution
</details>
Please look here: https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3441/6/display/redirect
**Test results:**
- PASSED: 3253
- FAILED: 1
Those are the test failures:
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3441/6/testReport/org.kie.kogito.quarkus.workflows/EventFlowIT/testNotStartingEvent/">org.kie.kogito.quarkus.workflows.EventFlowIT.testNotStartingEvent</a></summary>
1 expectation failed.<br/>Expected status code <201> but was <500>.<br/>
</details>
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix_#3413] Speed up businesskey query performance [incubator-kie-kogito-runtimes]
Posted by "fjtirado (via GitHub)" <gi...@apache.org>.
fjtirado commented on code in PR #3441:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3441#discussion_r1526039279
##########
addons/common/persistence/jdbc/src/main/java/org/kie/kogito/persistence/jdbc/GenericRepository.java:
##########
@@ -34,32 +34,35 @@
import javax.sql.DataSource;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
public class GenericRepository extends Repository {
private static final String PAYLOAD = "payload";
private static final String VERSION = "version";
- private static final Logger LOGGER = LoggerFactory.getLogger(GenericRepository.class);
-
private final DataSource dataSource;
public GenericRepository(DataSource dataSource) {
this.dataSource = dataSource;
}
@Override
- void insertInternal(String processId, String processVersion, UUID id, byte[] payload) {
+ void insertInternal(String processId, String processVersion, UUID id, byte[] payload, String businessKey) {
Review Comment:
The code is relying in on cascade, so deleting the process instance also delete the business key entry
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [Fix_#3413] Speed up businesskey query performance [incubator-kie-kogito-runtimes]
Posted by "kie-ci3 (via GitHub)" <gi...@apache.org>.
kie-ci3 commented on PR #3441:
URL: https://github.com/apache/incubator-kie-kogito-runtimes/pull/3441#issuecomment-1999673617
**PR job** `#12` was: **UNSTABLE**
Possible explanation: This should be test failures
<details>
<summary><b>Reproducer</b></summary>
build-chain build full_downstream -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-kogito-runtimes -u https://github.com/apache/incubator-kie-kogito-runtimes/pull/3441 --skipParallelCheckout
NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution
</details>
Please look here: https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3441/12/display/redirect
**Test results:**
- PASSED: 3253
- FAILED: 1
Those are the test failures:
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/kogito/job/main/job/pullrequest_jobs/job/kogito-runtimes-pr/job/PR-3441/12/testReport/org.kie.kogito.quarkus.workflows/EventFlowIT/testNotStartingEvent/">org.kie.kogito.quarkus.workflows.EventFlowIT.testNotStartingEvent</a></summary>
Condition with alias 'dead' didn't complete within 15 seconds because assertion condition defined as a org.kie.kogito.quarkus.workflows.AssuredTestUtils 1 expectation failed.<br/>Expected status code <404> but was <200>.<br/>.
</details>
--
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.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org