You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/02/17 17:30:48 UTC
[GitHub] [cassandra] bbotella opened a new pull request #1452: Fix Flaky testNoSuchRepairSessionAnticompaction
bbotella opened a new pull request #1452:
URL: https://github.com/apache/cassandra/pull/1452
Syncronizes session onSuccess handlePrepareSession for LocalSessions
patch by Bernardo Botella Corbi
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] yifan-c commented on a change in pull request #1452: Fix Flaky testNoSuchRepairSessionAnticompaction
Posted by GitBox <gi...@apache.org>.
yifan-c commented on a change in pull request #1452:
URL: https://github.com/apache/cassandra/pull/1452#discussion_r813401637
##########
File path: CHANGES.txt
##########
@@ -1,3 +1,5 @@
+Flaky testNoSuchRepairSessionAnticompaction (CASSANDRA-17335)
Review comment:
We do not normally add test-only fix to the changes list. The change in this patch is to fix a bug/race condition during repair. Can you rephrase the description? So it is more clear than "Flaky testNoSuchRepairSessionAnticompaction"
##########
File path: src/java/org/apache/cassandra/repair/consistent/LocalSessions.java
##########
@@ -829,14 +829,11 @@ public void onSuccess(@Nullable List<Void> result)
try
{
logger.info("Prepare phase for incremental repair session {} completed", sessionID);
- if (session.getState() != FAILED)
- setStateAndSave(session, PREPARED);
- else
+ if (!prepareSessionExceptFailed(session))
logger.info("Session {} failed before anticompaction completed", sessionID);
-
Message<PrepareConsistentResponse> message =
- Message.out(PREPARE_CONSISTENT_RSP,
- new PrepareConsistentResponse(sessionID, getBroadcastAddressAndPort(), session.getState() != FAILED));
+ Message.out(PREPARE_CONSISTENT_RSP,
+ new PrepareConsistentResponse(sessionID, getBroadcastAddressAndPort(), session.getState() != FAILED));
Review comment:
Can you restore the removed spaces? The change is not required for the fix.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bbotella commented on a change in pull request #1452: Fix Flaky testNoSuchRepairSessionAnticompaction
Posted by GitBox <gi...@apache.org>.
bbotella commented on a change in pull request #1452:
URL: https://github.com/apache/cassandra/pull/1452#discussion_r813405905
##########
File path: src/java/org/apache/cassandra/repair/consistent/LocalSessions.java
##########
@@ -829,14 +829,11 @@ public void onSuccess(@Nullable List<Void> result)
try
{
logger.info("Prepare phase for incremental repair session {} completed", sessionID);
- if (session.getState() != FAILED)
- setStateAndSave(session, PREPARED);
- else
+ if (!prepareSessionExceptFailed(session))
logger.info("Session {} failed before anticompaction completed", sessionID);
-
Message<PrepareConsistentResponse> message =
- Message.out(PREPARE_CONSISTENT_RSP,
- new PrepareConsistentResponse(sessionID, getBroadcastAddressAndPort(), session.getState() != FAILED));
+ Message.out(PREPARE_CONSISTENT_RSP,
+ new PrepareConsistentResponse(sessionID, getBroadcastAddressAndPort(), session.getState() != FAILED));
Review comment:
Yes. For some reason my IDE decided it was a good idea removing them. Sorry for this.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bbotella commented on a change in pull request #1452: Fix Flaky testNoSuchRepairSessionAnticompaction
Posted by GitBox <gi...@apache.org>.
bbotella commented on a change in pull request #1452:
URL: https://github.com/apache/cassandra/pull/1452#discussion_r809371999
##########
File path: src/java/org/apache/cassandra/repair/consistent/LocalSessions.java
##########
@@ -828,16 +828,19 @@ public void onSuccess(@Nullable List<Void> result)
{
try
{
- logger.info("Prepare phase for incremental repair session {} completed", sessionID);
- if (session.getState() != FAILED)
- setStateAndSave(session, PREPARED);
- else
- logger.info("Session {} failed before anticompaction completed", sessionID);
+ synchronized (session)
Review comment:
Both suggestions make sense. Posting a new commit to address them.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] yifan-c commented on a change in pull request #1452: Fix Flaky testNoSuchRepairSessionAnticompaction
Posted by GitBox <gi...@apache.org>.
yifan-c commented on a change in pull request #1452:
URL: https://github.com/apache/cassandra/pull/1452#discussion_r809342623
##########
File path: src/java/org/apache/cassandra/repair/consistent/LocalSessions.java
##########
@@ -828,16 +828,19 @@ public void onSuccess(@Nullable List<Void> result)
{
try
{
- logger.info("Prepare phase for incremental repair session {} completed", sessionID);
- if (session.getState() != FAILED)
- setStateAndSave(session, PREPARED);
- else
- logger.info("Session {} failed before anticompaction completed", sessionID);
+ synchronized (session)
Review comment:
The synchronized scope is a bit too large. I think you only need to make the two operations, `getState` and `setStateAndSave` atomic. You can make the synchronized block to cover the if..else.. statement.
One small step forward, you can probably add a `prepareSessionExceptFailed` method to wrap the get and set logic, e.g.
```java
/**
* return true if prepared. Return false if not, i.e. session faield
*/
boolean prepareSessionExceptFailed(LocalSession session)
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic closed pull request #1452: Fix race condition bug during local session repair
Posted by GitBox <gi...@apache.org>.
smiklosovic closed pull request #1452:
URL: https://github.com/apache/cassandra/pull/1452
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bbotella commented on a change in pull request #1452: Fix race condition bug during local session repair
Posted by GitBox <gi...@apache.org>.
bbotella commented on a change in pull request #1452:
URL: https://github.com/apache/cassandra/pull/1452#discussion_r813406486
##########
File path: CHANGES.txt
##########
@@ -1,3 +1,5 @@
+Flaky testNoSuchRepairSessionAnticompaction (CASSANDRA-17335)
Review comment:
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org