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 2020/07/15 13:15:13 UTC
[GitHub] [shardingsphere] SirMin opened a new pull request #6359: HintManager's getInstance returns the instance held by the current th…
SirMin opened a new pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359
…read or returns a new instance and corrects the junit test (#6342)
Fixes #6342.
Changes proposed in this pull request:
- If the current thread holds an instance of HintManager, return it, otherwise create a new instance, put it into ThreadLocal and, return it
- corrects the junit test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] SirMin edited a comment on pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
SirMin edited a comment on pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#issuecomment-667620255
My point is that these restrictions should be pointed out in the documentation.
The user has been able to obtain the HintManager object, but the user does not know whether the implementation details of the HintManager are reasonable? Because ShardingJdbc can no longer restrict users from using HintManager instances.My point is that users don’t need to know the specific implementation logic,but they should know how to implement it.
So the documentation of the user-oriented api is very important. However, there is no mention of the relationship between HintManager and threads in the HintManager documentation. Because the more intuitive relationship should be Connection and HintManager one-to-one correspondence or Statemant and HintManagerone-to-one correspondence.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero commented on a change in pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
tristaZero commented on a change in pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#discussion_r456193433
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/hint/HintManager.java
##########
@@ -43,15 +42,20 @@
private boolean masterRouteOnly;
/**
- * Get a new instance for {@code HintManager}.
+ * Get a instance for {@code HintManager}.
*
- * @return {@code HintManager} instance
+ * @return {@code HintManager} current thread hold or new instance
*/
public static HintManager getInstance() {
- Preconditions.checkState(null == HINT_MANAGER_HOLDER.get(), "Hint has previous value, please clear first.");
- HintManager result = new HintManager();
- HINT_MANAGER_HOLDER.set(result);
- return result;
+
+ final HintManager hintManager = HINT_MANAGER_HOLDER.get();
Review comment:
`final` is necessary or not? If it is optional, please remove it.
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/hint/HintManager.java
##########
@@ -43,15 +42,20 @@
private boolean masterRouteOnly;
/**
- * Get a new instance for {@code HintManager}.
+ * Get a instance for {@code HintManager}.
*
- * @return {@code HintManager} instance
+ * @return {@code HintManager} current thread hold or new instance
*/
public static HintManager getInstance() {
- Preconditions.checkState(null == HINT_MANAGER_HOLDER.get(), "Hint has previous value, please clear first.");
- HintManager result = new HintManager();
- HINT_MANAGER_HOLDER.set(result);
- return result;
+
Review comment:
Invalid blank line, please remove.
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/test/java/org/apache/shardingsphere/infra/hint/HintManagerTest.java
##########
@@ -20,17 +20,23 @@
import org.junit.Test;
import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
public final class HintManagerTest {
-
- @Test(expected = IllegalStateException.class)
+
+ @Test
public void assertGetInstanceTwice() {
try {
- HintManager.getInstance();
- HintManager.getInstance();
+ final HintManager instance = HintManager.getInstance();
Review comment:
final is necessary or not? If it is optional, please remove 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] dongzl commented on a change in pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
dongzl commented on a change in pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#discussion_r456195483
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/hint/HintManager.java
##########
@@ -31,7 +30,7 @@
*/
@NoArgsConstructor(access = AccessLevel.PRIVATE)
public final class HintManager implements AutoCloseable {
-
+
Review comment:
Please keep the white space.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero commented on pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
tristaZero commented on pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#issuecomment-661600503
@SirMin Hi
> The usage should be clearly indicated in the document,Instead of disabling this method because of some restrictions.
IMO, it is better to make users understand the usage of functions by functions themselves, like `newInstance()` is better than `getInstance()`.
> And I think it is not a good practice to use a big class to manage the entire Hint, because it is possible that I need to set the Hint separately.
Do you mean the following one?
` private static final ThreadLocal<HintManager> HINT_MANAGER_HOLDER = new ThreadLocal<>();`
Its detail is open to ShardingSphere, and nearly closed to users, preferably. Most of the users will not dig into the source codes.
> For example, the place where I get the database sharding value and the place where the table sharding value is not the same place, then I will not be able to Decide whether to clear
Yep, that the reason why I want to merge this PR at first!
My concern comes down to this: If we return the same instance of `hintManager`, how we can remind users that the different one will be returned in other threads.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero commented on pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
tristaZero commented on pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#issuecomment-660778759
@SirMin
> The current situation is that different threads are also returning different objects
It is supposed to be that. If not, it is unsafe when multiple threads modify it together.
> and without looking at the source code, I subconsciously think that this should be the object held by the current thread in each call,
I agree with you. `getInstance` always makes us believe it will return the same Instance. If we plan to return a new instance each time, `newInstance` will be better.
> And after calling get Instance, you must call close or clear, otherwise an error will be reported. Is this design reasonable?
`close()` function is supposed to be called by users. I am with you to some extent, but what the most concerned me that if `getInstance` always returns the same Instance, users are very likely to believe it works well even in child threads. But the fact is that a new one will be returned in other threads. How do we avoid this case?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero closed pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
tristaZero closed pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] SirMin commented on pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
SirMin commented on pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#issuecomment-667939364
Hi @tristaZero
>1. Maybe newInstance() is better to rename getInstance()
>2. As you said, we'd better add more java doc concerning `thread and its usage.
I agree more with the second. Because I think this API is user-oriented, it's best to keep it stable.
> Would you mind giving this class an improvement?
add new pr？
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero edited a comment on pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
tristaZero edited a comment on pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#issuecomment-659813723
Hi @SirMin
Very appreciated your PR!
Since this PR will change the public API to users, so we need to be serious about it.
@terrymanu gave his opinion that if users call `getInstance()` in different threads and get the same instance, it is not safe to do modification on the same one.
Besides, `getInstance()` is similar to `getConnection()`, which will return a new one to users.
Back to your need, maybe you can consider passing the `HintManager` by function parameters.
Therefore, I have to close this PR, and welcome your succeeding joining! :-)
Best,
Trista
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] SirMin commented on pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
SirMin commented on pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#issuecomment-667620255
My point is that these restrictions should be pointed out in the documentation。
The user has been able to obtain the HintManager object, but the user does not know whether the implementation details of the HintManager are reasonable?Because ShardingJdbc can no longer restrict users from using HintManager instances.My point is that users don’t need to know the specific implementation logic, but they should know how to implement it.
So the documentation of the user-oriented api is very important.However, there is no mention of the relationship between HintManager and threads in the HintManager documentation.Because the more intuitive relationship should be Connection and HintManager one-to-one correspondence or Statemant and HintManager one-to-one correspondence.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] SirMin commented on pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
SirMin commented on pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#issuecomment-660766118
The current situation is that different threads are also returning different objects, and without looking at the source code, I subconsciously think that this should be the object held by the current thread in each call, rather than multiple calls will throw an exception.
And after calling getInstance, you must call close or clear, otherwise an error will be reported. Is this design reasonable? Because under normal circumstances, closing or clearing should act on the resource, not on the method of obtaining the resource.
And if clear is not called, the next call is affected. Is this design a bit coupled?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero edited a comment on pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
tristaZero edited a comment on pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#issuecomment-659813723
Hi @SirMin
Very appreciated your PR!
The content of this PR is basically correct, and a unit test made it reliable. Plus, I left some comments, could you give them a second?
Trista
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero commented on pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
tristaZero commented on pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#issuecomment-668341121
The new function name of the first one will avoid users' confusion, IMO. The next release 5.x will have many changes for API. Maybe it is the best time to rename.
Yep, welcome your PR!
@SirMin
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] SirMin commented on pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
SirMin commented on pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#issuecomment-659969854
Hi @tristaZero HintManager's getInstance is thread safe beacuse the instance put to ThreadLocal, in different threads will not get the same instance.
The connection will be managed by most frameworks. If it is too costly to manage the HintManager like a connection, and we must clear it after setting up the HintManager, otherwise the next call will report an exception, so I cannot see the modification to get The HintManager held by the current thread has any bad
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] SirMin commented on pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
SirMin commented on pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#issuecomment-660978329
@tristaZero
I think users can decide how to use HintManager,The usage should be clearly indicated in the document,Instead of disabling this method because of some restrictions.
And I think it is not a good practice to use a big class to manage the entire Hint, because it is possible that I need to set the Hint separately. For example, the place where I get the database sharding value and the place where the table sharding value is not the same place, then I will not be able to Decide whether to clear
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero commented on pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
tristaZero commented on pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#issuecomment-667779776
Hi @Sir Thanks for your comment.
I partly agree with you. Would you mind giving this class an improvement?
1. Maybe `newInstance()` is better to rename `getInstance()`
2. As you said, we'd better add more java doc concerning `thread and its usage.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero commented on pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
tristaZero commented on pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#issuecomment-660058695
Hi @SirMin Thanks for your feedback. I would like to discuss this feature with you and @terrymanu. Let us light up our minds!
`ThreadLocal` is designed for `ShardingSphere` to get specific sharding values from users. But `an average user` will not care about `ThreadLocal ` or not, and he or she just wants to get one instance of `hintManger`.
If this function always returns the same one, it is likely to confuse users that `hintManger` is in`Singleton pattern`. Therefore, they might believe that the same `hintManger` will be returned in different threads, though.
One important point here is that how we tell users the information that `hintManger` is going to return the same one only in the current thread.
That is my opinion about this feature, what do you think? @SirMin @terrymanu
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] dongzl commented on a change in pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
dongzl commented on a change in pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#discussion_r456195606
##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/test/java/org/apache/shardingsphere/infra/hint/HintManagerTest.java
##########
@@ -20,17 +20,23 @@
import org.junit.Test;
import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
public final class HintManagerTest {
-
- @Test(expected = IllegalStateException.class)
+
Review comment:
Please keep the white space.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] SirMin edited a comment on pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
SirMin edited a comment on pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#issuecomment-667620255
My point is that these restrictions should be pointed out in the documentation.
The user has been able to obtain the HintManager object, but the user does not
know whether the implementation details of the HintManager are reasonable? Because
ShardingJdbc can no longer restrict users from using HintManager instances.My point
is that users don’t need to know the specific implementation logic, but they should
know how to implement it.
So the documentation of the user-oriented api is very important. However, there
is no mention of the relationship between HintManager and threads in the HintManager
documentation. Because the more intuitive relationship should be Connection and
HintManager one-to-one correspondence or Statemant and HintManager one-to-one
correspondence.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] SirMin commented on pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
SirMin commented on pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#issuecomment-671151505
ok, I will add new pr
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero commented on pull request #6359: HintManager's getInstance returns the instance held by the current th…
Posted by GitBox <gi...@apache.org>.
tristaZero commented on pull request #6359:
URL: https://github.com/apache/shardingsphere/pull/6359#issuecomment-659813723
Hi @SirMin
Very appreciated your PR!
The content of this PR is basically correct, and a unit test made it reliable. Plus, I left some comments, could you them a second?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org