-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix workflow manager concurrency #97
Conversation
while [ $? -ne 1 ]; do
./gradlew test --tests *WorkflowManagerSpec --debug
done |
87a0faf
to
e72171c
Compare
@sagarsane @jdigger @jazeren1 ready for review. I ran 4000 ./gradlew check on both master, and this branch overnight; and neither had any concurrency test failures. However, I ran 200 ./gradlew build on both master, and this branch; and master had 12 failures out of 200, and this branch had 0 failures out of 200. I modified the script above a bit to test SUCCESS=0
FAILURE=0
while [ 1 ]; do
./gradlew build --no-daemon
if [ $? == 1 ]; then
FAILURE=$(( FAILURE + 1))
else
SUCCESS=$(( SUCCESS + 1))
fi
echo "FAILURE count : ${FAILURE}"
echo "SUCCESS count : ${SUCCESS}"
sleep 1
done |
import java.util.concurrent.ConcurrentHashMap | ||
import java.util.concurrent.atomic.AtomicInteger | ||
import java.util.concurrent.atomic.AtomicInteger as JobUseCount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems super weird but I understand why this will be useful 😸 from code understanding perspective.
So the first time I ran this in IntelliJ, I got the above error 😢 .. I tried running this multiple times after that (using Gradle and otherwise) and it passed ... :/ |
Whaa? |
Don't suppose you captured a thread dump, did you? :S |
I know 😭 I haven't been able to break it since then. I am going to try though. |
Ok got it to break again. Here are the details from the console :
|
@sagarsane that's gotta be from master :) My log statements are different, and plus I renamed LauncherConfigsMap to workflowSemaphore |
@masroormohammed , do you mind testing this from a functional perspective? @sagarsane tells me you understand this part. Looks good from my angle |
+1 CR and test |
Thanks Masroor! |
WIP @sagarsane @jdigger @jazeren1