You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2022/08/09 08:38:31 UTC
[GitHub] [shardingsphere] tuichenchuxin opened a new pull request, #20020: remove TrafficContextHolder.java use sql session to replace
tuichenchuxin opened a new pull request, #20020:
URL: https://github.com/apache/shardingsphere/pull/20020
For #20005.
Changes proposed in this pull request:
- remove TrafficContextHolder.java
--
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@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tuichenchuxin commented on a diff in pull request #20020: remove TrafficContextHolder.java use sql session to replace
Posted by GitBox <gi...@apache.org>.
tuichenchuxin commented on code in PR #20020:
URL: https://github.com/apache/shardingsphere/pull/20020#discussion_r942101059
##########
shardingsphere-jdbc/shardingsphere-jdbc-core/src/main/java/org/apache/shardingsphere/driver/jdbc/core/statement/ShardingSphereStatement.java:
##########
@@ -185,19 +184,22 @@ private static SQLFederationDeciderContext decide(final LogicSQL logicSQL, final
return deciderEngine.decide(logicSQL, database);
}
- private TrafficContext getTrafficContext(final LogicSQL logicSQL) {
- TrafficContext result = TrafficContextHolder.get().orElseGet(() -> createTrafficContext(logicSQL));
- if (connection.isHoldTransaction()) {
- TrafficContextHolder.set(result);
+ private Optional<String> getInstanceIdAndSave(final LogicSQL logicSQL) {
+ Optional<String> trafficInstanceId = connection.getSqlSession().getTrafficInstanceId();
+ if (!trafficInstanceId.isPresent()) {
+ trafficInstanceId = getTrafficInstanceId(logicSQL);
}
- return result;
+ if (connection.isHoldTransaction() && trafficInstanceId.isPresent()) {
+ connection.getSqlSession().setTrafficInstanceId(trafficInstanceId.get());
+ }
+ return trafficInstanceId;
Review Comment:
done.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.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] strongduanmu commented on a diff in pull request #20020: remove TrafficContextHolder.java use sql session to replace
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on code in PR #20020:
URL: https://github.com/apache/shardingsphere/pull/20020#discussion_r942092711
##########
shardingsphere-jdbc/shardingsphere-jdbc-core/src/main/java/org/apache/shardingsphere/driver/jdbc/core/statement/ShardingSphereStatement.java:
##########
@@ -185,19 +184,22 @@ private static SQLFederationDeciderContext decide(final LogicSQL logicSQL, final
return deciderEngine.decide(logicSQL, database);
}
- private TrafficContext getTrafficContext(final LogicSQL logicSQL) {
- TrafficContext result = TrafficContextHolder.get().orElseGet(() -> createTrafficContext(logicSQL));
- if (connection.isHoldTransaction()) {
- TrafficContextHolder.set(result);
+ private Optional<String> getInstanceIdAndSave(final LogicSQL logicSQL) {
+ Optional<String> trafficInstanceId = connection.getSqlSession().getTrafficInstanceId();
+ if (!trafficInstanceId.isPresent()) {
+ trafficInstanceId = getTrafficInstanceId(logicSQL);
}
- return result;
+ if (connection.isHoldTransaction() && trafficInstanceId.isPresent()) {
+ connection.getSqlSession().setTrafficInstanceId(trafficInstanceId.get());
+ }
+ return trafficInstanceId;
Review Comment:
Please rename trafficInstanceId to result.
##########
shardingsphere-jdbc/shardingsphere-jdbc-core/src/main/java/org/apache/shardingsphere/driver/jdbc/core/statement/ShardingSpherePreparedStatement.java:
##########
@@ -211,9 +210,9 @@ public ResultSet executeQuery() throws SQLException {
}
clearPrevious();
LogicSQL logicSQL = createLogicSQL();
- trafficContext = getTrafficContext(logicSQL);
- if (trafficContext.isMatchTraffic()) {
- JDBCExecutionUnit executionUnit = createTrafficExecutionUnit(trafficContext, logicSQL);
+ trafficInstanceId = getTrafficInstanceIdAndSave(logicSQL).orElse(null);
Review Comment:
Can we return Optional here?
##########
shardingsphere-jdbc/shardingsphere-jdbc-core/src/main/java/org/apache/shardingsphere/driver/jdbc/core/statement/ShardingSphereStatement.java:
##########
@@ -157,9 +156,9 @@ public ResultSet executeQuery(final String sql) throws SQLException {
ResultSet result;
try {
LogicSQL logicSQL = createLogicSQL(sql);
- trafficContext = getTrafficContext(logicSQL);
- if (trafficContext.isMatchTraffic()) {
- JDBCExecutionUnit executionUnit = createTrafficExecutionUnit(trafficContext, logicSQL);
+ trafficInstanceId = getInstanceIdAndSave(logicSQL).orElse(null);
Review Comment:
getInstanceIdAndSet may be 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.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] strongduanmu merged pull request #20020: remove TrafficContextHolder.java use sql session to replace
Posted by GitBox <gi...@apache.org>.
strongduanmu merged PR #20020:
URL: https://github.com/apache/shardingsphere/pull/20020
--
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@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] strongduanmu commented on a diff in pull request #20020: remove TrafficContextHolder.java use sql session to replace
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on code in PR #20020:
URL: https://github.com/apache/shardingsphere/pull/20020#discussion_r941929259
##########
shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/session/traffic/TrafficSessionContext.java:
##########
@@ -15,43 +15,35 @@
* limitations under the License.
*/
-package org.apache.shardingsphere.traffic.context;
+package org.apache.shardingsphere.infra.session.traffic;
-import lombok.AccessLevel;
-import lombok.NoArgsConstructor;
+import lombok.Getter;
+import lombok.Setter;
import java.util.Optional;
/**
- * Hold traffic context for current thread.
+ * Traffic session context.
*/
-@NoArgsConstructor(access = AccessLevel.PRIVATE)
-public final class TrafficContextHolder {
+@Getter
+public final class TrafficSessionContext {
- private static final ThreadLocal<TrafficContext> TRAFFIC_CONTEXT = new ThreadLocal<>();
-
- /**
- * Set traffic context.
- *
- * @param trafficContext traffic context
- */
- public static void set(final TrafficContext trafficContext) {
- TRAFFIC_CONTEXT.set(trafficContext);
- }
+ @Setter
+ private TrafficContextAware trafficContext;
/**
* Get traffic context.
- *
+ *
* @return traffic context
*/
- public static Optional<TrafficContext> get() {
- return Optional.ofNullable(TRAFFIC_CONTEXT.get());
+ public Optional<TrafficContextAware> getTrafficContext() {
+ return Optional.ofNullable(trafficContext);
}
/**
- * Remove traffic context.
+ * Clear.
*/
- public static void remove() {
- TRAFFIC_CONTEXT.remove();
+ public void clear() {
Review Comment:
Implement AutoClosable interface?
##########
shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/session/SQLSession.java:
##########
@@ -15,28 +15,16 @@
* limitations under the License.
*/
-package org.apache.shardingsphere.traffic.context;
+package org.apache.shardingsphere.infra.session;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.mockito.Mock;
-import org.mockito.junit.MockitoJUnitRunner;
+import lombok.Getter;
+import org.apache.shardingsphere.infra.session.traffic.TrafficSessionContext;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-
-@RunWith(MockitoJUnitRunner.class)
-public final class TrafficContextHolderTest {
-
- @Mock
- private TrafficContext trafficContext;
+/**
+ * SQL session.
+ */
+@Getter
+public final class SQLSession {
- @Test
- public void assertTrafficContextHolder() {
- assertFalse(TrafficContextHolder.get().isPresent());
- TrafficContextHolder.set(trafficContext);
- assertTrue(TrafficContextHolder.get().isPresent());
- TrafficContextHolder.remove();
- assertFalse(TrafficContextHolder.get().isPresent());
- }
+ private final TrafficSessionContext trafficSessionContext = new TrafficSessionContext();
Review Comment:
Do you think TrafficSQLSession is better?
##########
shardingsphere-jdbc/shardingsphere-jdbc-core/src/main/java/org/apache/shardingsphere/driver/jdbc/core/statement/ShardingSpherePreparedStatement.java:
##########
@@ -250,9 +249,9 @@ private JDBCExecutionUnit createTrafficExecutionUnit(final TrafficContext traffi
}
private TrafficContext getTrafficContext(final LogicSQL logicSQL) {
- TrafficContext result = TrafficContextHolder.get().orElseGet(() -> createTrafficContext(logicSQL));
+ TrafficContext result = (TrafficContext) connection.getSqlSession().getTrafficSessionContext().getTrafficContext().orElseGet(() -> createTrafficContext(logicSQL));
Review Comment:
Can we remove trafficSessionContext?
##########
shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/session/traffic/TrafficSessionContext.java:
##########
@@ -15,43 +15,35 @@
* limitations under the License.
*/
-package org.apache.shardingsphere.traffic.context;
+package org.apache.shardingsphere.infra.session.traffic;
-import lombok.AccessLevel;
-import lombok.NoArgsConstructor;
+import lombok.Getter;
+import lombok.Setter;
import java.util.Optional;
/**
- * Hold traffic context for current thread.
+ * Traffic session context.
*/
-@NoArgsConstructor(access = AccessLevel.PRIVATE)
-public final class TrafficContextHolder {
+@Getter
+public final class TrafficSessionContext {
- private static final ThreadLocal<TrafficContext> TRAFFIC_CONTEXT = new ThreadLocal<>();
-
- /**
- * Set traffic context.
- *
- * @param trafficContext traffic context
- */
- public static void set(final TrafficContext trafficContext) {
- TRAFFIC_CONTEXT.set(trafficContext);
- }
+ @Setter
+ private TrafficContextAware trafficContext;
Review Comment:
What's the purpose of TrafficContextAware?
--
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@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tuichenchuxin commented on a diff in pull request #20020: remove TrafficContextHolder.java use sql session to replace
Posted by GitBox <gi...@apache.org>.
tuichenchuxin commented on code in PR #20020:
URL: https://github.com/apache/shardingsphere/pull/20020#discussion_r942078805
##########
shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/session/SQLSession.java:
##########
@@ -15,28 +15,16 @@
* limitations under the License.
*/
-package org.apache.shardingsphere.traffic.context;
+package org.apache.shardingsphere.infra.session;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.mockito.Mock;
-import org.mockito.junit.MockitoJUnitRunner;
+import lombok.Getter;
+import org.apache.shardingsphere.infra.session.traffic.TrafficSessionContext;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-
-@RunWith(MockitoJUnitRunner.class)
-public final class TrafficContextHolderTest {
-
- @Mock
- private TrafficContext trafficContext;
+/**
+ * SQL session.
+ */
+@Getter
+public final class SQLSession {
- @Test
- public void assertTrafficContextHolder() {
- assertFalse(TrafficContextHolder.get().isPresent());
- TrafficContextHolder.set(trafficContext);
- assertTrue(TrafficContextHolder.get().isPresent());
- TrafficContextHolder.remove();
- assertFalse(TrafficContextHolder.get().isPresent());
- }
+ private final TrafficSessionContext trafficSessionContext = new TrafficSessionContext();
Review Comment:
Thanks.Done.
--
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@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org