You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "xu1009 (via GitHub)" <gi...@apache.org> on 2023/03/28 04:13:26 UTC
[GitHub] [skywalking-java] xu1009 opened a new pull request, #489: support mysql when useServerPrepStmts
xu1009 opened a new pull request, #489:
URL: https://github.com/apache/skywalking-java/pull/489
### <support mysql trace sql param when useServerPrepStmts>
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<10589>.
- [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking-java/blob/main/CHANGES.md).
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng merged pull request #489: support mysql when useServerPrepStmts
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #489:
URL: https://github.com/apache/skywalking-java/pull/489
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #489: support mysql when useServerPrepStmts
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #489:
URL: https://github.com/apache/skywalking-java/pull/489#discussion_r1150608602
##########
test/plugin/scenarios/mysql-scenario/src/main/java/org/apache/skywalking/apm/testcase/mysql/SQLExecutor.java:
##########
@@ -53,28 +54,28 @@ public void insertData(String sql, String id, String value) throws SQLException
public void dropTable(String sql) throws SQLException {
executeStatement(sql);
}
-
+
Review Comment:
This seems unexpected change?
##########
test/plugin/scenarios/mysql-scenario/configuration.yml:
##########
@@ -30,3 +30,4 @@ dependencies:
environment:
- MYSQL_ROOT_PASSWORD=root
- MYSQL_DATABASE=test
+
Review Comment:
Please revert this 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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #489: support mysql when useServerPrepStmts
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #489:
URL: https://github.com/apache/skywalking-java/pull/489#discussion_r1150626012
##########
test/plugin/scenarios/mysql-scenario/src/main/java/org/apache/skywalking/apm/testcase/mysql/SQLExecutor.java:
##########
@@ -53,28 +54,28 @@ public void insertData(String sql, String id, String value) throws SQLException
public void dropTable(String sql) throws SQLException {
executeStatement(sql);
}
-
+
Review Comment:
@xu1009 This has not been reverted.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #489: support mysql when useServerPrepStmts
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #489:
URL: https://github.com/apache/skywalking-java/pull/489#discussion_r1150694451
##########
CHANGES.md:
##########
@@ -23,6 +23,7 @@ Release Notes.
* Support keep trace profiling when cross-thread.
* Fix unexpected whitespace of the command catalogs in several Redis plugins.
* Fix a thread leak in `SamplingService` when updated sampling policy in the runtime.
+* Support MySQL plugin trace sql parameters when useServerPrepStmts
Review Comment:
```suggestion
* Support MySQL plugin trace SQL parameters when useServerPrepStmts
```
##########
CHANGES.md:
##########
@@ -23,6 +23,7 @@ Release Notes.
* Support keep trace profiling when cross-thread.
* Fix unexpected whitespace of the command catalogs in several Redis plugins.
* Fix a thread leak in `SamplingService` when updated sampling policy in the runtime.
+* Support MySQL plugin trace sql parameters when useServerPrepStmts
Review Comment:
```suggestion
* Support MySQL plugin tracing SQL parameters when useServerPrepStmts
```
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #489: support mysql when useServerPrepStmts
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #489:
URL: https://github.com/apache/skywalking-java/pull/489#discussion_r1150106889
##########
test/plugin/scenarios/mysql-scenario/src/main/resources/jdbc.properties:
##########
@@ -13,6 +13,6 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
-mysql.url=jdbc:mysql://mysql-server:3306/test?serverTimezone=CST&enabledTLSProtocols=TLSv1.2&useSSL=false
+mysql.url=jdbc:mysql://mysql-server:3306/test?serverTimezone=CST&enabledTLSProtocols=TLSv1.2&useSSL=false&useServerPrepStmts=true
Review Comment:
@xu1009 ☝️
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #489: support mysql when useServerPrepStmts
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #489:
URL: https://github.com/apache/skywalking-java/pull/489#discussion_r1150640968
##########
test/plugin/scenarios/mysql-scenario/src/main/java/org/apache/skywalking/apm/testcase/mysql/SQLExecutor.java:
##########
@@ -53,28 +54,28 @@ public void insertData(String sql, String id, String value) throws SQLException
public void dropTable(String sql) throws SQLException {
executeStatement(sql);
}
-
+
Review Comment:
No matter what it is. Please keep unchanged. Git is able to revert this file before the change.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #489: support mysql when useServerPrepStmts
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #489:
URL: https://github.com/apache/skywalking-java/pull/489#discussion_r1150045395
##########
test/plugin/scenarios/mysql-scenario/src/main/resources/jdbc.properties:
##########
@@ -13,6 +13,6 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
-mysql.url=jdbc:mysql://mysql-server:3306/test?serverTimezone=CST&enabledTLSProtocols=TLSv1.2&useSSL=false
+mysql.url=jdbc:mysql://mysql-server:3306/test?serverTimezone=CST&enabledTLSProtocols=TLSv1.2&useSSL=false&useServerPrepStmts=true
Review Comment:
Is this going to override the PreparedStatement cases?
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] xu1009 commented on a diff in pull request #489: support mysql when useServerPrepStmts
Posted by "xu1009 (via GitHub)" <gi...@apache.org>.
xu1009 commented on code in PR #489:
URL: https://github.com/apache/skywalking-java/pull/489#discussion_r1150428682
##########
test/plugin/scenarios/mysql-scenario/src/main/resources/jdbc.properties:
##########
@@ -13,6 +13,6 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
-mysql.url=jdbc:mysql://mysql-server:3306/test?serverTimezone=CST&enabledTLSProtocols=TLSv1.2&useSSL=false
+mysql.url=jdbc:mysql://mysql-server:3306/test?serverTimezone=CST&enabledTLSProtocols=TLSv1.2&useSSL=false&useServerPrepStmts=true
Review Comment:
yes, PreparedStatement case will be overrided, create new test for useServerPrepStmts?
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #489: support mysql when useServerPrepStmts
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #489:
URL: https://github.com/apache/skywalking-java/pull/489#discussion_r1150572254
##########
test/plugin/scenarios/mysql-scenario/src/main/resources/jdbc.properties:
##########
@@ -13,6 +13,6 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
-mysql.url=jdbc:mysql://mysql-server:3306/test?serverTimezone=CST&enabledTLSProtocols=TLSv1.2&useSSL=false
+mysql.url=jdbc:mysql://mysql-server:3306/test?serverTimezone=CST&enabledTLSProtocols=TLSv1.2&useSSL=false&useServerPrepStmts=true
Review Comment:
Generally, as this has been tested, I think we should be OK to revert the changes. To keep the duplicate cases for just this one thing seems a little overreactive.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #489: support mysql when useServerPrepStmts
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #489:
URL: https://github.com/apache/skywalking-java/pull/489#discussion_r1150572803
##########
test/plugin/scenarios/mysql-scenario/src/main/resources/jdbc.properties:
##########
@@ -13,6 +13,6 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
-mysql.url=jdbc:mysql://mysql-server:3306/test?serverTimezone=CST&enabledTLSProtocols=TLSv1.2&useSSL=false
+mysql.url=jdbc:mysql://mysql-server:3306/test?serverTimezone=CST&enabledTLSProtocols=TLSv1.2&useSSL=false&useServerPrepStmts=true
Review Comment:
I feel confident to accept this feature w/o test case change.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] xu1009 commented on a diff in pull request #489: support mysql when useServerPrepStmts
Posted by "xu1009 (via GitHub)" <gi...@apache.org>.
xu1009 commented on code in PR #489:
URL: https://github.com/apache/skywalking-java/pull/489#discussion_r1150637000
##########
test/plugin/scenarios/mysql-scenario/src/main/java/org/apache/skywalking/apm/testcase/mysql/SQLExecutor.java:
##########
@@ -53,28 +54,28 @@ public void insertData(String sql, String id, String value) throws SQLException
public void dropTable(String sql) throws SQLException {
executeStatement(sql);
}
-
+
Review Comment:
it seems some Newline character different
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org