You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2017/08/25 22:55:54 UTC
[kudu-CR] KUDU-1544: Race in Java client's AsyncKuduSession.apply()
Hello Jean-Daniel Cryans, Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/7839
to review the following change.
Change subject: KUDU-1544: Race in Java client's AsyncKuduSession.apply()
......................................................................
KUDU-1544: Race in Java client's AsyncKuduSession.apply()
This fixes a potential race in AsyncKuduSession.apply by acquiring the
notification deferred early. See the JIRA for details about the race.
Acquiring the notification deferred early is always safe, the only
downside is that clients may be spuriously notified that there is buffer
space available when in fact there is not.
Change-Id: I1791f5ed51e2d79cd9e9bcbf83e9b30eede871b4
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
1 file changed, 4 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7839/1
--
To view, visit http://gerrit.cloudera.org:8080/7839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1791f5ed51e2d79cd9e9bcbf83e9b30eede871b4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
[kudu-CR] KUDU-1544: Race in Java client's AsyncKuduSession.apply()
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1544: Race in Java client's AsyncKuduSession.apply()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7839/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
Line 549: Deferred<Void> notification = flushNotification.get();
> Any particular reason not to call flushNotification.get() just once a scope
nope! done.
--
To view, visit http://gerrit.cloudera.org:8080/7839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1791f5ed51e2d79cd9e9bcbf83e9b30eede871b4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1544: Race in Java client's AsyncKuduSession.apply()
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.
Change subject: KUDU-1544: Race in Java client's AsyncKuduSession.apply()
......................................................................
KUDU-1544: Race in Java client's AsyncKuduSession.apply()
This fixes a potential race in AsyncKuduSession.apply by acquiring the
notification deferred early. See the JIRA for details about the race.
Acquiring the notification deferred early is always safe, the only
downside is that clients may be spuriously notified that there is buffer
space available when in fact there is not.
Change-Id: I1791f5ed51e2d79cd9e9bcbf83e9b30eede871b4
Reviewed-on: http://gerrit.cloudera.org:8080/7839
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
1 file changed, 4 insertions(+), 3 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I1791f5ed51e2d79cd9e9bcbf83e9b30eede871b4
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1544: Race in Java client's AsyncKuduSession.apply()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1544: Race in Java client's AsyncKuduSession.apply()
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1791f5ed51e2d79cd9e9bcbf83e9b30eede871b4
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1544: Race in Java client's AsyncKuduSession.apply()
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: KUDU-1544: Race in Java client's AsyncKuduSession.apply()
......................................................................
Patch Set 1: Code-Review+2
Did you loop TestAsyncKuduSession?
--
To view, visit http://gerrit.cloudera.org:8080/7839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1791f5ed51e2d79cd9e9bcbf83e9b30eede871b4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-1544: Race in Java client's AsyncKuduSession.apply()
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Jean-Daniel Cryans, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7839
to look at the new patch set (#3).
Change subject: KUDU-1544: Race in Java client's AsyncKuduSession.apply()
......................................................................
KUDU-1544: Race in Java client's AsyncKuduSession.apply()
This fixes a potential race in AsyncKuduSession.apply by acquiring the
notification deferred early. See the JIRA for details about the race.
Acquiring the notification deferred early is always safe, the only
downside is that clients may be spuriously notified that there is buffer
space available when in fact there is not.
Change-Id: I1791f5ed51e2d79cd9e9bcbf83e9b30eede871b4
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
1 file changed, 4 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7839/3
--
To view, visit http://gerrit.cloudera.org:8080/7839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1791f5ed51e2d79cd9e9bcbf83e9b30eede871b4
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1544: Race in Java client's AsyncKuduSession.apply()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1544: Race in Java client's AsyncKuduSession.apply()
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7839/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java:
Line 549: Deferred<Void> notification = flushNotification.get();
Any particular reason not to call flushNotification.get() just once a scope or two or three higher, so that the same local can be reused in the activeBuffer != null case?
Line 605: null, operation, flushNotification.get());
Here as well?
--
To view, visit http://gerrit.cloudera.org:8080/7839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1791f5ed51e2d79cd9e9bcbf83e9b30eede871b4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes