Conversation
linghengqian
left a comment
linghengqian
left a comment
There was a problem hiding this comment.
I glanced at your commit history. Out of curiosity, is your development equipment unable to use bash or PowerShell 7?
./mvnw spotless:apply -Pcheck -T1C
./mvnw checkstyle:check -Pcheck -T1C
|
HI, @linghengqian Thank you for providing the Spotless and CheckStyle commands earlier. I had been running such commands with mvn and kept encountering failures, but after switching to mvnw, it succeeded. |
linghengqian
left a comment
linghengqian
left a comment
There was a problem hiding this comment.
Based on https://github.com/apache/shardingsphere/actions/workflows/ci.yml , the CI for the master branch is functioning normally. If the CI fails, it is either due to changes you introduced or because you did not rebase or merge the latest changes from the master branch. Refer to https://github.com/apache/shardingsphere/actions/runs/21193761740/job/60967190368?pr=37791#step:7:16641 for further details.
[INFO] Running org.apache.shardingsphere.mode.metadata.manager.resource.StorageUnitManagerTest
02:05:12.057 [main] ERROR org.apache.shardingsphere.mode.metadata.manager.resource.StorageUnitManager - Alter database: foo_db register storage unit failed.
java.sql.SQLException: register error
at org.apache.shardingsphere.mode.metadata.manager.resource.StorageUnitManagerTest.lambda$assertRegisterLogsErrorWhenSQLException$1(StorageUnitManagerTest.java:88)
at org.mockito.internal.stubbing.StubbedInvocationMatcher.answer(StubbedInvocationMatcher.java:42)
at org.mockito.internal.handler.MockHandlerImpl.handle(MockHandlerImpl.java:103)
at org.mockito.internal.handler.NullResultGuardian.handle(NullResultGuardian.java:29)
at org.mockito.internal.handler.InvocationNotifierHandler.handle(InvocationNotifierHandler.java:34)
at org.mockito.internal.creation.bytebuddy.MockMethodInterceptor.doIntercept(MockMethodInterceptor.java:82)
at org.mockito.internal.creation.bytebuddy.MockMethodAdvice.handle(MockMethodAdvice.java:134)
at org.apache.shardingsphere.mode.metadata.manager.resource.ResourceSwitchManager.switchByRegisterStorageUnit(ResourceSwitchManager.java:53)
at org.apache.shardingsphere.mode.metadata.manager.resource.StorageUnitManager.register(StorageUnitManager.java:65)
at org.apache.shardingsphere.mode.metadata.manager.resource.StorageUnitManagerTest.lambda$assertRegisterLogsErrorWhenSQLException$2(StorageUnitManagerTest.java:90)
at org.junit.jupiter.api.AssertDoesNotThrow.assertDoesNotThrow(AssertDoesNotThrow.java:49)
at org.junit.jupiter.api.AssertDoesNotThrow.assertDoesNotThrow(AssertDoesNotThrow.java:36)
at org.junit.jupiter.api.Assertions.assertDoesNotThrow(Assertions.java:3199)
at org.apache.shardingsphere.mode.metadata.manager.resource.StorageUnitManagerTest.assertRegisterLogsErrorWhenSQLException(StorageUnitManagerTest.java:90)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:786)
at org.junit.platform.commons.support.ReflectionSupport.invokeMethod(ReflectionSupport.java:514)
at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131)
at org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:161)
at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:152)
at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:91)
at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(InterceptingExecutableInvoker.java:112)
at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.lambda$invoke$0(InterceptingExecutableInvoker.java:94)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37)
at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:93)
at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:87)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$4(TestMethodTestDescriptor.java:221)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:217)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:159)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:70)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:157)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:147)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:145)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:144)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:101)
at java.util.ArrayList.forEach(ArrayList.java:1259)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:161)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:147)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:145)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:144)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:101)
at java.util.ArrayList.forEach(ArrayList.java:1259)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:161)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:147)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:145)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:144)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:101)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:35)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:54)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.executeEngine(EngineExecutionOrchestrator.java:230)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.failOrExecuteEngine(EngineExecutionOrchestrator.java:204)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:172)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:101)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:64)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:150)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:63)
at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:109)
at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:91)
at org.junit.platform.launcher.core.DelegatingLauncher.execute(DelegatingLauncher.java:47)
at org.junit.platform.launcher.core.InterceptingLauncher.lambda$execute$1(InterceptingLauncher.java:39)
at org.junit.platform.launcher.core.ClasspathAlignmentCheckingLauncherInterceptor.intercept(ClasspathAlignmentCheckingLauncherInterceptor.java:25)
at org.junit.platform.launcher.core.InterceptingLauncher.execute(InterceptingLauncher.java:38)
at org.junit.platform.launcher.core.DelegatingLauncher.execute(DelegatingLauncher.java:47)
at org.apache.maven.surefire.junitplatform.LazyLauncher.execute(LazyLauncher.java:56)
at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.execute(JUnitPlatformProvider.java:184)
at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.invokeAllTests(JUnitPlatformProvider.java:148)
at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.invoke(JUnitPlatformProvider.java:122)
at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:385)
at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:162)
at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:507)
at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)
There was a problem hiding this comment.
or advise on how to proceed?
- You might see a checkbox like this in the description of the current PR.
[ √] I have passed maven check locally :
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e
- However, if you look at the GitHub Actions logs, you'll see that all the commands there can be run in bash.
- If you're using Windows 11 , you might be able to use bash by using devcontainers.
- If you are unable to install WSL2 to use devcontainers, you can also use GitHub Codespace to start a remote development environment. Since E2E testing uses testcontainers, you always need a development environment that can run Linux containers.
- Please note that I am not discriminating against PowerShell 7. For PowerShell 7, you can install the Docker CLI on Windows and then connect to dockerd in wsl2. This is also what rancher desktop or docker desktop do.
linghengqian
left a comment
linghengqian
left a comment
There was a problem hiding this comment.
LGTM. However, since I am unsure of the details of the original issue, I need another committer to review it.
|
thanks |
There was a problem hiding this comment.
The SPI-based parameter replayer idea is a good direction, and the new unit tests around the replayer are aligned with the goal of fixing the Oracle BLOB issue—please keep that.
Change requests:
- Batch execution still falls back to setObject. ShardingSpherePreparedStatement.addBatch clears parameterRecords (jdbc/src/main/java/org/apache/shardingsphere/driver/jdbc/core/statement/ShardingSpherePreparedStatement.java:320), so during batch replay the code only has raw values and uses the default replayer. Oracle BLOB/InputStream inserts in batch will still fail with “invalid column type.” Please persist the per-batch setter metadata (e.g., store PreparedStatementParameter per batch) and use it when replaying batches.
Please address these points before merging.
|
HI,@linghengqian |
Out of curiosity, what error log did you see? I don't think there are any known issues with CI. |
The CI is correct Please refer to C:\Users\daiq\IdeaProjects\shardingsphere\agent\core\target\surefire-reports for the individual test results. Please refer to C:\Users\daiq\IdeaProjects\shardingsphere\agent\core\target\surefire-reports for the individual test results. C:\Users\daiq\IdeaProjects\shardingsphere> ` |
|
I can't see which unit tests the I think you're not using bash, so some unit tests might not be handling non-Linux systems. |
terrymanu
left a comment
terrymanu
left a comment
There was a problem hiding this comment.
What’s good:
- Splitting parameter replay into an SPI with an Oracle-specific implementation is the right direction, and the new tests cover the Oracle setter branches.
Issues to address:
- jdbc/src/main/java/org/apache/shardingsphere/driver/jdbc/adapter/AbstractPreparedStatementAdapter.java: batch parameter recording only keeps the first batch’s setterMethodType and length. Subsequent batches reuse those when findParameterRecord runs, so if later batches call different setters (e.g., setNull, setBytes) or lengths, replay uses the wrong setter/length, risking “Invalid column type” or incorrect writes. Please persist per-batch parameter records (including setter/length) or carry that info alongside parameterSets so each batch replays with its own metadata.
- Oracle SPI may not load in the JDBC artifact: jdbc/pom.xml does not pull in shardingsphere-database-connector-oracle, so the default replayer is likely used and still calls setObject. Please add the dependency or otherwise ensure the Oracle SPI is on the JDBC classpath; without it, #37504 is unresolved.
- getDatabaseType now calls preparedStatement.getConnection().getMetaData().getURL() on every replay and passes it to DatabaseTypeFactory.get. If the driver returns null/non-standard URLs, this can throw and is a new failure mode. Add guarding or cache a known database type.
Missing coverage:
- Add a batch scenario test with at least two batches using different setters/lengths to verify replay uses the correct setter and length per batch.
Please fix the above and we can re-review.
|
thanks your review. The changes are becoming more and more extensive and complex. I'm curious how you all are testing these changes. I integrated version 5.5.3-SNAPSHOT into my test project, but it failed to connect to Oracle properly. Only version 5.5.1 works correctly. If I have to submit a PR online every time I make a change and wait for your review, wouldn't that be very cumbersome? |
There was a problem hiding this comment.
If I have to submit a PR online every time I make a change and wait for your review, wouldn't that be very cumbersome?
It's really up to you. Every CI file has an on.workflow_dispatch tag, so you can manually start CI in your forked repository.
Your downstream test projects can also be configured for CI, whether it's GitHub Actions, GitLab Runner, or Gitea Runner. It's entirely up to you; you just need to pull code from your forked repository on GitHub and cache it in the /root/.m2 directory through a specific linux runner for your downstream projects. Some people call this process Git Ops.
If you feel that GitHub Actions' reliance on github.com makes it too easy for Microsoft to leak your privacy, and you are worried that Microsoft will use your privacy to make money, you can also use https://github.com/nektos/act to run the GitHub Actions runner locally. Whether or not you do this depends entirely on how much you hate Microsoft.
Before submitting a PR, you need to perform tests, which include connecting to Oracle databases.
I sorry for any inconvenience this may cause, but this is the standard collaboration model in the open-source community. |
|
1.I wrote the code based on version 5.5.1, then used a local test project of my own for integration testing. After the test succeeded, I migrated the code from 5.5.1 to 5.5.3-SNAPSHOT. However, I can’t verify the correctness of the code after migration in 5.5.3-SNAPSHOT on my own. 2.Why didn’t I directly use 5.5.3-SNAPSHOT for my local test project? Because 5.5.3-SNAPSHOT always has weird issues that prevent it from working properly. Earlier, when I integrated it into the project, it failed to work with Oracle (I can’t find the screenshot now). Today, it fails to install, as shown in the image below: 3.After making modifications on 5.5.1, I tested with the following test case and confirmed that the SQL can be inserted into the database normally. The code is as follows: 4.Earlier, I said, 【If I have to submit a PR online every time I make a change and wait for your review, wouldn't that be very cumbersome?】 I didn’t mean that I find it troublesome—what I was worried about is that you might find it cumbersome. If you don’t mind the back-and-forth, I’ll continue fixing this bug. If you think it’s taking too long with repeated revisions and it’s delaying your review time, please let me know. |
linghengqian
left a comment
linghengqian
left a comment
There was a problem hiding this comment.
Because 5.5.3-SNAPSHOT always has weird issues that prevent it from working properly.
- If you can reproduce the issue on the master branch, you can open a new issue. It worked fine when I tested features unrelated to the ShardingSphere agent on version 5.5.3-SNAPSHOT at https://github.com/linghengqian/shardingsphere-agent-master-test .
If you don’t mind the back-and-forth, I’ll continue fixing this bug.
- Personally, I don't care, but it means that CI won't be started at any time, since currently only humans can approve the start of CI.
I tried integrating 5.5.3-SNAPSHOT into my test project again last night, and this time it successfully ran on Oracle. |
|
Any update? |
|
I made some revisions and completed the final submission before the Spring Festival.I haven’t touched this matter since returning to work, and I will only continue working on it on Saturdays and Sundays from now on. |
3 participants
Fixes #37504
Changes proposed in this pull request:
Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.Fix "Invalid column type" error when inserting BLOB type via ojdbc6; use SPI to add Oracle-specific handling for BLOB and other related parameter types