You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by GitBox <gi...@apache.org> on 2022/06/24 11:06:29 UTC
[GitHub] [incubator-pegasus] WHBANG opened a new pull request, #1019: Java client scan add has next method
WHBANG opened a new pull request, #1019:
URL: https://github.com/apache/incubator-pegasus/pull/1019
### What problem does this PR solve? <!--add issue link with summary if exists-->
https://github.com/apache/incubator-pegasus/issues/1018
### What is changed and how does it work?
### Checklist <!--REMOVE the items that are not applicable-->
##### Tests <!-- At least one of them must be included. -->
- Unit test
- Integration test
- Manual test (add detailed scripts or steps below)
- No code
##### Code changes
- Has exported function/method change
- Has exported variable/fields change
- Has interface methods change
- Has persistent data change
##### Side effects
- Possible performance regression
- Increased code complexity
- Breaking backward compatibility
##### Related changes
- Need to cherry-pick to the release branch
- Need to update the documentation
- Need to be included in the release note
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1019: Java client scan add hasNext method
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1019:
URL: https://github.com/apache/incubator-pegasus/pull/1019#discussion_r906946986
##########
java-client/src/main/java/org/apache/pegasus/client/PegasusScanner.java:
##########
@@ -88,9 +88,27 @@ public PegasusScanner(
_needCheckHash = needCheckHash;
_incomplete = false;
_fullScan = fullScan;
+ _nextItem = null;
Review Comment:
The global variable will cause `thread-safe` problem, in origin code, I remember it will `lock` before fetch `next`: https://github.com/WHBANG/incubator-pegasus/blob/bd099a9c963de8de4cd8ad4c80ca6bb9653d24cd/java-client/src/main/java/org/apache/pegasus/client/PegasusScanner.java#L125
Please check and make sure the `refactor` is `thread-safe`
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1019: refactor: Java client scan add hasNext method
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1019:
URL: https://github.com/apache/incubator-pegasus/pull/1019#discussion_r907215778
##########
java-client/src/main/java/org/apache/pegasus/client/PegasusScanner.java:
##########
@@ -88,9 +88,31 @@ public PegasusScanner(
_needCheckHash = needCheckHash;
_incomplete = false;
_fullScan = fullScan;
+ _nextItem = null;
+ }
+
+ public boolean hasNext() throws PException {
+ synchronized (_nextItemLock) {
+ if (_incomplete) {
+ return false;
+ }
+ if (_rpcRunning && !_encounterError) {
+ return !_promises.isEmpty();
Review Comment:
`_promises.isEmpty()` but just because the running rpc has't completed. that is to say, when the rpc compeleted, the `_promises` is not empty.
The above case has problem?
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1019: feat(java-client): Add hasNext() for PegasusScannerInterface
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1019:
URL: https://github.com/apache/incubator-pegasus/pull/1019#discussion_r909186933
##########
java-client/src/main/java/org/apache/pegasus/client/PegasusScanner.java:
##########
@@ -88,9 +88,27 @@ public PegasusScanner(
_needCheckHash = needCheckHash;
_incomplete = false;
_fullScan = fullScan;
+ _nextItem = null;
+ }
+
+ public boolean hasNext() throws PException {
+ synchronized (_promisesLock) {
+ if (_nextItem != null) {
+ return true;
+ }
+ }
+ _nextItem = next();
Review Comment:
It's not thread safe in some cases since `_nextItem` will be overwritten.
```
time thread1 thread2
t1 hasNext hasNext
t2 _nextItem = a
t3 _nextItem = b
t4 next -> b
t5 next -> nullptr
```
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 merged pull request #1019: feat(java-client): Add hasNext() for PegasusScannerInterface
Posted by GitBox <gi...@apache.org>.
acelyc111 merged PR #1019:
URL: https://github.com/apache/incubator-pegasus/pull/1019
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1019: refactor: Java client scan add hasNext method
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1019:
URL: https://github.com/apache/incubator-pegasus/pull/1019#discussion_r908343401
##########
java-client/src/main/java/org/apache/pegasus/client/PegasusScanner.java:
##########
@@ -88,9 +88,31 @@ public PegasusScanner(
_needCheckHash = needCheckHash;
_incomplete = false;
_fullScan = fullScan;
+ _nextItem = null;
+ }
+
+ public boolean hasNext() throws PException {
+ synchronized (_nextItemLock) {
+ if (_incomplete) {
+ return false;
+ }
+ if (_rpcRunning && !_encounterError) {
+ return !_promises.isEmpty();
+ } else {
+ _nextItem = next();
+ return _nextItem != null;
+ }
+ }
Review Comment:
> but if hasNext is called many times individually, it will cause the iterator to move, which is not desirable
This case is right, so you may need update it like:
```java
public boolean hasNext() throws PException {
synchronized (_nextItemLock) {
if (_nextItem != null) {
return true;
}
}
_nextItem = next();
return _nextItem != null;
}
```
##########
java-client/src/main/java/org/apache/pegasus/client/PegasusScanner.java:
##########
@@ -88,9 +88,31 @@ public PegasusScanner(
_needCheckHash = needCheckHash;
_incomplete = false;
_fullScan = fullScan;
+ _nextItem = null;
+ }
+
+ public boolean hasNext() throws PException {
+ synchronized (_nextItemLock) {
+ if (_incomplete) {
+ return false;
+ }
+ if (_rpcRunning && !_encounterError) {
+ return !_promises.isEmpty();
+ } else {
+ _nextItem = next();
+ return _nextItem != null;
+ }
+ }
Review Comment:
> but if hasNext is called many times individually, it will cause the iterator to move, which is not desirable
This case is right, so you may need update it like:
```java
public boolean hasNext() throws PException {
synchronized (_nextItemLock) {
if (_nextItem != null) {
return true;
}
}
_nextItem = next();
return _nextItem != null;
}
```
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] WHBANG commented on a diff in pull request #1019: refactor: Java client scan add hasNext method
Posted by GitBox <gi...@apache.org>.
WHBANG commented on code in PR #1019:
URL: https://github.com/apache/incubator-pegasus/pull/1019#discussion_r907129025
##########
java-client/src/main/java/org/apache/pegasus/client/PegasusScanner.java:
##########
@@ -88,9 +88,31 @@ public PegasusScanner(
_needCheckHash = needCheckHash;
_incomplete = false;
_fullScan = fullScan;
+ _nextItem = null;
+ }
+
+ public boolean hasNext() throws PException {
+ synchronized (_nextItemLock) {
+ if (_incomplete) {
+ return false;
+ }
+ if (_rpcRunning && !_encounterError) {
+ return !_promises.isEmpty();
+ } else {
+ _nextItem = next();
+ return _nextItem != null;
+ }
+ }
}
public Pair<Pair<byte[], byte[]>, byte[]> next() throws PException {
+ synchronized (_nextItemLock) {
Review Comment:
good idea. i agree
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] WHBANG commented on a diff in pull request #1019: refactor: Java client scan add hasNext method
Posted by GitBox <gi...@apache.org>.
WHBANG commented on code in PR #1019:
URL: https://github.com/apache/incubator-pegasus/pull/1019#discussion_r907073433
##########
java-client/src/main/java/org/apache/pegasus/client/PegasusScanner.java:
##########
@@ -88,9 +88,27 @@ public PegasusScanner(
_needCheckHash = needCheckHash;
_incomplete = false;
_fullScan = fullScan;
+ _nextItem = null;
Review Comment:
OK, i will add _nextItemLock to make _nextItem thread-safe
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1019: refactor: Java client scan add hasNext method
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1019:
URL: https://github.com/apache/incubator-pegasus/pull/1019#discussion_r907106023
##########
java-client/src/main/java/org/apache/pegasus/client/PegasusScanner.java:
##########
@@ -88,9 +88,31 @@ public PegasusScanner(
_needCheckHash = needCheckHash;
_incomplete = false;
_fullScan = fullScan;
+ _nextItem = null;
+ }
+
+ public boolean hasNext() throws PException {
+ synchronized (_nextItemLock) {
+ if (_incomplete) {
+ return false;
+ }
+ if (_rpcRunning && !_encounterError) {
+ return !_promises.isEmpty();
+ } else {
+ _nextItem = next();
+ return _nextItem != null;
+ }
+ }
}
public Pair<Pair<byte[], byte[]>, byte[]> next() throws PException {
+ synchronized (_nextItemLock) {
Review Comment:
shouldn't use new `lock`, you should use `_promisesLock`
##########
java-client/src/main/java/org/apache/pegasus/client/PegasusScanner.java:
##########
@@ -88,9 +88,31 @@ public PegasusScanner(
_needCheckHash = needCheckHash;
_incomplete = false;
_fullScan = fullScan;
+ _nextItem = null;
+ }
+
+ public boolean hasNext() throws PException {
+ synchronized (_nextItemLock) {
+ if (_incomplete) {
+ return false;
+ }
+ if (_rpcRunning && !_encounterError) {
+ return !_promises.isEmpty();
+ } else {
+ _nextItem = next();
+ return _nextItem != null;
+ }
+ }
Review Comment:
I think you don't have to make such complex, just like follow seem to be ok:
```java
public boolean hasNext() throws PException {
_nextItem = next();
return _nextItem != null;
}
```
`next()` method include enough logic to judge current status and make sure `thread-safe`.
That is to say `_incomplete`, `_rpcRunning && !_encounterError` should throw exception but return fasle
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] WHBANG commented on a diff in pull request #1019: refactor: Java client scan add hasNext method
Posted by GitBox <gi...@apache.org>.
WHBANG commented on code in PR #1019:
URL: https://github.com/apache/incubator-pegasus/pull/1019#discussion_r907126232
##########
java-client/src/main/java/org/apache/pegasus/client/PegasusScanner.java:
##########
@@ -88,9 +88,31 @@ public PegasusScanner(
_needCheckHash = needCheckHash;
_incomplete = false;
_fullScan = fullScan;
+ _nextItem = null;
+ }
+
+ public boolean hasNext() throws PException {
+ synchronized (_nextItemLock) {
+ if (_incomplete) {
+ return false;
+ }
+ if (_rpcRunning && !_encounterError) {
+ return !_promises.isEmpty();
+ } else {
+ _nextItem = next();
+ return _nextItem != null;
+ }
+ }
Review Comment:
Yes, but if hasNext is called many times individually, it will cause the iterator to move, which is not desirable.
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] WHBANG commented on a diff in pull request #1019: refactor: Java client scan add hasNext method
Posted by GitBox <gi...@apache.org>.
WHBANG commented on code in PR #1019:
URL: https://github.com/apache/incubator-pegasus/pull/1019#discussion_r907295907
##########
java-client/src/main/java/org/apache/pegasus/client/PegasusScanner.java:
##########
@@ -88,9 +88,31 @@ public PegasusScanner(
_needCheckHash = needCheckHash;
_incomplete = false;
_fullScan = fullScan;
+ _nextItem = null;
+ }
+
+ public boolean hasNext() throws PException {
+ synchronized (_nextItemLock) {
+ if (_incomplete) {
+ return false;
+ }
+ if (_rpcRunning && !_encounterError) {
+ return !_promises.isEmpty();
Review Comment:
when the rpc compeleted, the scan is not compeleted, but the key-value is cached on the _kvs and promise queues, obtained in turn through next(), and _promises.removeFirst()
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 closed pull request #1019: feat(java-client): Java client scan add hasNext method
Posted by GitBox <gi...@apache.org>.
acelyc111 closed pull request #1019: feat(java-client): Java client scan add hasNext method
URL: https://github.com/apache/incubator-pegasus/pull/1019
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] WHBANG closed pull request #1019: feat(java-client): Java client scan add hasNext method
Posted by GitBox <gi...@apache.org>.
WHBANG closed pull request #1019: feat(java-client): Java client scan add hasNext method
URL: https://github.com/apache/incubator-pegasus/pull/1019
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org