You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by "stoty (via GitHub)" <gi...@apache.org> on 2023/11/02 08:52:33 UTC
[PR] Implement Statement.closeOnCompletion() and fix related close() bugs [phoenix]
stoty opened a new pull request, #1729:
URL: https://github.com/apache/phoenix/pull/1729
(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: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7095 Implement Statement.closeOnCompletion() and fix related close() bugs [phoenix]
Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on code in PR #1729:
URL: https://github.com/apache/phoenix/pull/1729#discussion_r1393697723
##########
phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixStatement.java:
##########
@@ -330,161 +333,162 @@ protected PhoenixResultSet executeQuery(final CompilableStatement stmt, final Qu
}
private PhoenixResultSet executeQuery(final CompilableStatement stmt,
- final boolean doRetryOnMetaNotFoundError, final QueryLogger queryLogger, final boolean noCommit) throws SQLException {
+ final boolean doRetryOnMetaNotFoundError, final QueryLogger queryLogger,
+ final boolean noCommit) throws SQLException {
GLOBAL_SELECT_SQL_COUNTER.increment();
try {
- return CallRunner
- .run(new CallRunner.CallableThrowable<PhoenixResultSet, SQLException>() {
- @Override public PhoenixResultSet call() throws SQLException {
- final long startTime = EnvironmentEdgeManager.currentTimeMillis();
- boolean success = false;
- boolean pointLookup = false;
- String tableName = null;
- PhoenixResultSet rs = null;
- try {
- PhoenixConnection conn = getConnection();
- conn.checkOpen();
-
- if (conn.getQueryServices().isUpgradeRequired() && !conn
- .isRunningUpgrade()
- && stmt.getOperation() != Operation.UPGRADE) {
- throw new UpgradeRequiredException();
- }
- QueryPlan
- plan =
- stmt.compilePlan(PhoenixStatement.this,
- Sequence.ValueOp.VALIDATE_SEQUENCE);
- // Send mutations to hbase, so they are visible to subsequent reads.
- // Use original plan for data table so that data and immutable indexes will be sent
- // TODO: for joins, we need to iterate through all tables, but we need the original table,
- // not the projected table, so plan.getContext().getResolver().getTables() won't work.
- if (plan.getTableRef() != null
- && plan.getTableRef().getTable() != null && !Strings
- .isNullOrEmpty(
- plan.getTableRef().getTable().getPhysicalName()
- .toString())) {
- tableName = plan.getTableRef().getTable().getPhysicalName()
- .toString();
- }
- if (plan.getContext().getScanRanges().isPointLookup()) {
- pointLookup = true;
- }
- Iterator<TableRef> tableRefs = plan.getSourceRefs().iterator();
- connection.getMutationState().sendUncommitted(tableRefs);
- plan =
- connection.getQueryServices().getOptimizer()
- .optimize(PhoenixStatement.this, plan);
- // this will create its own trace internally, so we don't wrap this
- // whole thing in tracing
- ResultIterator resultIterator = plan.iterator();
+ return CallRunner.run(
Review Comment:
It's the usual fight with the usless 120 character line width limit.
Since the code was already not well formatted, I may as well revert 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: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7095 Implement Statement.closeOnCompletion() and fix related close() bugs [phoenix]
Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on code in PR #1729:
URL: https://github.com/apache/phoenix/pull/1729#discussion_r1393697723
##########
phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixStatement.java:
##########
@@ -330,161 +333,162 @@ protected PhoenixResultSet executeQuery(final CompilableStatement stmt, final Qu
}
private PhoenixResultSet executeQuery(final CompilableStatement stmt,
- final boolean doRetryOnMetaNotFoundError, final QueryLogger queryLogger, final boolean noCommit) throws SQLException {
+ final boolean doRetryOnMetaNotFoundError, final QueryLogger queryLogger,
+ final boolean noCommit) throws SQLException {
GLOBAL_SELECT_SQL_COUNTER.increment();
try {
- return CallRunner
- .run(new CallRunner.CallableThrowable<PhoenixResultSet, SQLException>() {
- @Override public PhoenixResultSet call() throws SQLException {
- final long startTime = EnvironmentEdgeManager.currentTimeMillis();
- boolean success = false;
- boolean pointLookup = false;
- String tableName = null;
- PhoenixResultSet rs = null;
- try {
- PhoenixConnection conn = getConnection();
- conn.checkOpen();
-
- if (conn.getQueryServices().isUpgradeRequired() && !conn
- .isRunningUpgrade()
- && stmt.getOperation() != Operation.UPGRADE) {
- throw new UpgradeRequiredException();
- }
- QueryPlan
- plan =
- stmt.compilePlan(PhoenixStatement.this,
- Sequence.ValueOp.VALIDATE_SEQUENCE);
- // Send mutations to hbase, so they are visible to subsequent reads.
- // Use original plan for data table so that data and immutable indexes will be sent
- // TODO: for joins, we need to iterate through all tables, but we need the original table,
- // not the projected table, so plan.getContext().getResolver().getTables() won't work.
- if (plan.getTableRef() != null
- && plan.getTableRef().getTable() != null && !Strings
- .isNullOrEmpty(
- plan.getTableRef().getTable().getPhysicalName()
- .toString())) {
- tableName = plan.getTableRef().getTable().getPhysicalName()
- .toString();
- }
- if (plan.getContext().getScanRanges().isPointLookup()) {
- pointLookup = true;
- }
- Iterator<TableRef> tableRefs = plan.getSourceRefs().iterator();
- connection.getMutationState().sendUncommitted(tableRefs);
- plan =
- connection.getQueryServices().getOptimizer()
- .optimize(PhoenixStatement.this, plan);
- // this will create its own trace internally, so we don't wrap this
- // whole thing in tracing
- ResultIterator resultIterator = plan.iterator();
+ return CallRunner.run(
Review Comment:
It's the usual fight with the useless 120 character line width limit.
Since the code was already not well formatted, I may as well revert 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: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7095 Implement Statement.closeOnCompletion() and fix related close() bugs [phoenix]
Posted by "richardantal (via GitHub)" <gi...@apache.org>.
richardantal commented on code in PR #1729:
URL: https://github.com/apache/phoenix/pull/1729#discussion_r1392575597
##########
phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java:
##########
@@ -720,17 +717,17 @@ public void clearWarnings() throws SQLException {
}
private void closeStatements() throws SQLException {
- List<? extends PhoenixStatement> statements = this.statements;
- // create new list to prevent close of statements
- // from modifying this list.
- this.statements = Lists.newArrayList();
try {
mutationState.rollback();
} catch (SQLException e) {
// ignore any exceptions while rolling back
} finally {
try {
- SQLCloseables.closeAll(statements);
+ // create new list to prevent close of statements from modifying this collection.
+ // TODO This could be optimized out by decoupling closing the stmt and removing it
+ // from the connection.
+ List<? extends PhoenixStatement> statementsCopy = new ArrayList<>(this.statements);
Review Comment:
Is there a reason why the statements got changed to HashSet?
This copy is a list and it is not consistent this way.
--
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: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement Statement.closeOnCompletion() and fix related close() bugs [phoenix]
Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1729:
URL: https://github.com/apache/phoenix/pull/1729#issuecomment-1790310300
No tests yet, I just want to see if this causes any regressions.
--
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: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7095 Implement Statement.closeOnCompletion() and fix related close() bugs [phoenix]
Posted by "richardantal (via GitHub)" <gi...@apache.org>.
richardantal commented on code in PR #1729:
URL: https://github.com/apache/phoenix/pull/1729#discussion_r1392747337
##########
phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixStatement.java:
##########
@@ -330,161 +333,162 @@ protected PhoenixResultSet executeQuery(final CompilableStatement stmt, final Qu
}
private PhoenixResultSet executeQuery(final CompilableStatement stmt,
- final boolean doRetryOnMetaNotFoundError, final QueryLogger queryLogger, final boolean noCommit) throws SQLException {
+ final boolean doRetryOnMetaNotFoundError, final QueryLogger queryLogger,
+ final boolean noCommit) throws SQLException {
GLOBAL_SELECT_SQL_COUNTER.increment();
try {
- return CallRunner
- .run(new CallRunner.CallableThrowable<PhoenixResultSet, SQLException>() {
- @Override public PhoenixResultSet call() throws SQLException {
- final long startTime = EnvironmentEdgeManager.currentTimeMillis();
- boolean success = false;
- boolean pointLookup = false;
- String tableName = null;
- PhoenixResultSet rs = null;
- try {
- PhoenixConnection conn = getConnection();
- conn.checkOpen();
-
- if (conn.getQueryServices().isUpgradeRequired() && !conn
- .isRunningUpgrade()
- && stmt.getOperation() != Operation.UPGRADE) {
- throw new UpgradeRequiredException();
- }
- QueryPlan
- plan =
- stmt.compilePlan(PhoenixStatement.this,
- Sequence.ValueOp.VALIDATE_SEQUENCE);
- // Send mutations to hbase, so they are visible to subsequent reads.
- // Use original plan for data table so that data and immutable indexes will be sent
- // TODO: for joins, we need to iterate through all tables, but we need the original table,
- // not the projected table, so plan.getContext().getResolver().getTables() won't work.
- if (plan.getTableRef() != null
- && plan.getTableRef().getTable() != null && !Strings
- .isNullOrEmpty(
- plan.getTableRef().getTable().getPhysicalName()
- .toString())) {
- tableName = plan.getTableRef().getTable().getPhysicalName()
- .toString();
- }
- if (plan.getContext().getScanRanges().isPointLookup()) {
- pointLookup = true;
- }
- Iterator<TableRef> tableRefs = plan.getSourceRefs().iterator();
- connection.getMutationState().sendUncommitted(tableRefs);
- plan =
- connection.getQueryServices().getOptimizer()
- .optimize(PhoenixStatement.this, plan);
- // this will create its own trace internally, so we don't wrap this
- // whole thing in tracing
- ResultIterator resultIterator = plan.iterator();
+ return CallRunner.run(
Review Comment:
I've checked this part of code locally because the here on the website it is really hard to review reformatted code.
As I see there is only a minor change in this part that setLastResultSet is not called.
To be honest I would not do this reformatting because it changes the git log/blame, makes it hard to search.
--
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: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7095 Implement Statement.closeOnCompletion() and fix related close() bugs [phoenix]
Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on code in PR #1729:
URL: https://github.com/apache/phoenix/pull/1729#discussion_r1393696844
##########
phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixConnection.java:
##########
@@ -720,17 +717,17 @@ public void clearWarnings() throws SQLException {
}
private void closeStatements() throws SQLException {
- List<? extends PhoenixStatement> statements = this.statements;
- // create new list to prevent close of statements
- // from modifying this list.
- this.statements = Lists.newArrayList();
try {
mutationState.rollback();
} catch (SQLException e) {
// ignore any exceptions while rolling back
} finally {
try {
- SQLCloseables.closeAll(statements);
+ // create new list to prevent close of statements from modifying this collection.
+ // TODO This could be optimized out by decoupling closing the stmt and removing it
+ // from the connection.
+ List<? extends PhoenixStatement> statementsCopy = new ArrayList<>(this.statements);
Review Comment:
Connections may have several open statements.
Removing closed connections from an ArrayList is O(n), while remving them from a Hash is O(1).
(Though for a few open statements this doesn't matter much)
But you are right, since closeAll() eventually removes the elements from the copy, it's better to use a HashSet for the copy as well.
--
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: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7095 Implement Statement.closeOnCompletion() and fix related close() bugs [phoenix]
Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty closed pull request #1729: PHOENIX-7095 Implement Statement.closeOnCompletion() and fix related close() bugs
URL: https://github.com/apache/phoenix/pull/1729
--
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: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7095 Implement Statement.closeOnCompletion() and fix related close() bugs [phoenix]
Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on code in PR #1729:
URL: https://github.com/apache/phoenix/pull/1729#discussion_r1393720720
##########
phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixStatement.java:
##########
@@ -330,161 +333,162 @@ protected PhoenixResultSet executeQuery(final CompilableStatement stmt, final Qu
}
private PhoenixResultSet executeQuery(final CompilableStatement stmt,
- final boolean doRetryOnMetaNotFoundError, final QueryLogger queryLogger, final boolean noCommit) throws SQLException {
+ final boolean doRetryOnMetaNotFoundError, final QueryLogger queryLogger,
+ final boolean noCommit) throws SQLException {
GLOBAL_SELECT_SQL_COUNTER.increment();
try {
- return CallRunner
- .run(new CallRunner.CallableThrowable<PhoenixResultSet, SQLException>() {
- @Override public PhoenixResultSet call() throws SQLException {
- final long startTime = EnvironmentEdgeManager.currentTimeMillis();
- boolean success = false;
- boolean pointLookup = false;
- String tableName = null;
- PhoenixResultSet rs = null;
- try {
- PhoenixConnection conn = getConnection();
- conn.checkOpen();
-
- if (conn.getQueryServices().isUpgradeRequired() && !conn
- .isRunningUpgrade()
- && stmt.getOperation() != Operation.UPGRADE) {
- throw new UpgradeRequiredException();
- }
- QueryPlan
- plan =
- stmt.compilePlan(PhoenixStatement.this,
- Sequence.ValueOp.VALIDATE_SEQUENCE);
- // Send mutations to hbase, so they are visible to subsequent reads.
- // Use original plan for data table so that data and immutable indexes will be sent
- // TODO: for joins, we need to iterate through all tables, but we need the original table,
- // not the projected table, so plan.getContext().getResolver().getTables() won't work.
- if (plan.getTableRef() != null
- && plan.getTableRef().getTable() != null && !Strings
- .isNullOrEmpty(
- plan.getTableRef().getTable().getPhysicalName()
- .toString())) {
- tableName = plan.getTableRef().getTable().getPhysicalName()
- .toString();
- }
- if (plan.getContext().getScanRanges().isPointLookup()) {
- pointLookup = true;
- }
- Iterator<TableRef> tableRefs = plan.getSourceRefs().iterator();
- connection.getMutationState().sendUncommitted(tableRefs);
- plan =
- connection.getQueryServices().getOptimizer()
- .optimize(PhoenixStatement.this, plan);
- // this will create its own trace internally, so we don't wrap this
- // whole thing in tracing
- ResultIterator resultIterator = plan.iterator();
+ return CallRunner.run(
Review Comment:
Addressed both concerns.
--
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: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org