You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "tdiesler (via GitHub)" <gi...@apache.org> on 2024/04/16 11:38:24 UTC
[PR] (fix#5315): concurrent map read and map write [camel-k]
tdiesler opened a new pull request, #5375:
URL: https://github.com/apache/camel-k/pull/5375
ctrlClientWrapper synchronizes all Reader/Writer access across clients of this type
https://github.com/kubernetes-sigs/controller-runtime/issues/2782
https://github.com/apache/camel-k/issues/5315
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (fix#5315): concurrent map read and map write [camel-k]
Posted by "tdiesler (via GitHub)" <gi...@apache.org>.
tdiesler commented on PR #5375:
URL: https://github.com/apache/camel-k/pull/5375#issuecomment-2063309453
Ready to merge
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (fix#5315): concurrent map read and map write [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on PR #5375:
URL: https://github.com/apache/camel-k/pull/5375#issuecomment-2066158810
@squakez hopefully it is fixed. we really know when the error is not coming back as it is an error of flaky nature and happens quite randomly. Let's hope the best and cross fingers for the next couple of days with workflow runs
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (fix#5315): concurrent map read and map write [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd commented on code in PR #5375:
URL: https://github.com/apache/camel-k/pull/5375#discussion_r1570300295
##########
pkg/client/ctrlclient_wrapper.go:
##########
Review Comment:
I would like to see this wrapper being added in the E2E test package only. We do only have this concurrency errors during E2E tests. I would like to avoid adding this to production code just because of the test errors.
Also we need to check if that setup has impact on the E2E test execution time. If we loose the execution speed in the E2E tests with that the solution might be to not run tests in parallel at all.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (fix#5315): concurrent map read and map write [camel-k]
Posted by "tdiesler (via GitHub)" <gi...@apache.org>.
tdiesler commented on code in PR #5375:
URL: https://github.com/apache/camel-k/pull/5375#discussion_r1568403594
##########
script/Makefile:
##########
@@ -267,19 +267,25 @@ test-common: do-build
test-smoke: do-build
FAILED=0; STAGING_RUNTIME_REPO="$(STAGING_RUNTIME_REPO)"; \
go test -timeout 10m -v ./e2e/common/main_test.go -tags=integration $(TEST_INTEGRATION_COMMON_LANG_RUN) $(GOTESTFMT) || FAILED=1; \
- go test -timeout 30m -v ./e2e/common/languages -tags=integration -parallel=$(TEST_COMMON_PARALLEL_COUNT) $(TEST_INTEGRATION_COMMON_LANG_RUN) $(GOTESTFMT) || FAILED=1; \
- go test -timeout 30m -v \
- ./e2e/common/misc/cron_test.go \
- ./e2e/common/misc/kamelet_test.go \
- ./e2e/common/misc/pipe_test.go \
- -tags=integration -parallel=$(TEST_COMMON_PARALLEL_COUNT) $(TEST_INTEGRATION_COMMON_LANG_RUN) $(GOTESTFMT) || FAILED=1; \
- go test -timeout 30m -v \
- ./e2e/common/traits/camel_test.go \
- ./e2e/common/traits/container_test.go \
- ./e2e/common/traits/openapi_test.go \
- ./e2e/common/traits/service_test.go \
- -tags=integration -parallel=$(TEST_COMMON_PARALLEL_COUNT) $(TEST_INTEGRATION_COMMON_LANG_RUN) $(GOTESTFMT) || FAILED=1; \
- exit $${FAILED}
+ if [ $$FAILED -eq 0 ]; then \
Review Comment:
Yes, this allows a contributer to 'quickly' verify that a change set is ready for PR. IMHO, it is sufficient that one test fails for an outcome that is no different when subsequent fail as well. Not skipping ahead would mean to always wait +30min
If you prefer, we could have something similar to this
https://maven.apache.org/surefire/maven-surefire-plugin/examples/skip-after-failure.html
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (fix#5315): concurrent map read and map write [camel-k]
Posted by "tdiesler (via GitHub)" <gi...@apache.org>.
tdiesler commented on PR #5375:
URL: https://github.com/apache/camel-k/pull/5375#issuecomment-2062041974
Ready to merge
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (fix#5315): concurrent map read and map write [camel-k]
Posted by "tdiesler (via GitHub)" <gi...@apache.org>.
tdiesler commented on PR #5375:
URL: https://github.com/apache/camel-k/pull/5375#issuecomment-2064516916
Ready to merge
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (fix#5315): concurrent map read and map write [camel-k]
Posted by "tdiesler (via GitHub)" <gi...@apache.org>.
tdiesler commented on code in PR #5375:
URL: https://github.com/apache/camel-k/pull/5375#discussion_r1570977964
##########
pkg/client/ctrlclient_wrapper.go:
##########
Review Comment:
It may be the case that the singleton approach to apis.AddToScheme() fixes it and we don't need the wrapper at all.
Could we pls run this PR again?
##########
pkg/client/ctrlclient_wrapper.go:
##########
Review Comment:
It may be the case that the singleton approach to apis.AddToScheme() fixes it and we don't need the wrapper at all. Could we pls run this PR again?
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (fix#5315): concurrent map read and map write [camel-k]
Posted by "tdiesler (via GitHub)" <gi...@apache.org>.
tdiesler commented on PR #5375:
URL: https://github.com/apache/camel-k/pull/5375#issuecomment-2064384281
Locally, I can confirm that wrapping the test client and syncing all read/write access with RLock takes 0.5% longer (10sec) for the smoke tests i.e. no measurable difference
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (fix#5315): concurrent map read and map write [camel-k]
Posted by "christophd (via GitHub)" <gi...@apache.org>.
christophd merged PR #5375:
URL: https://github.com/apache/camel-k/pull/5375
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (fix#5315): concurrent map read and map write [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #5375:
URL: https://github.com/apache/camel-k/pull/5375#issuecomment-2066099018
If this has fixed stability of test, we should backport to 2.3 please. Thanks.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (fix#5315): concurrent map read and map write [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5375:
URL: https://github.com/apache/camel-k/pull/5375#discussion_r1568360845
##########
script/Makefile:
##########
@@ -267,19 +267,25 @@ test-common: do-build
test-smoke: do-build
FAILED=0; STAGING_RUNTIME_REPO="$(STAGING_RUNTIME_REPO)"; \
go test -timeout 10m -v ./e2e/common/main_test.go -tags=integration $(TEST_INTEGRATION_COMMON_LANG_RUN) $(GOTESTFMT) || FAILED=1; \
- go test -timeout 30m -v ./e2e/common/languages -tags=integration -parallel=$(TEST_COMMON_PARALLEL_COUNT) $(TEST_INTEGRATION_COMMON_LANG_RUN) $(GOTESTFMT) || FAILED=1; \
- go test -timeout 30m -v \
- ./e2e/common/misc/cron_test.go \
- ./e2e/common/misc/kamelet_test.go \
- ./e2e/common/misc/pipe_test.go \
- -tags=integration -parallel=$(TEST_COMMON_PARALLEL_COUNT) $(TEST_INTEGRATION_COMMON_LANG_RUN) $(GOTESTFMT) || FAILED=1; \
- go test -timeout 30m -v \
- ./e2e/common/traits/camel_test.go \
- ./e2e/common/traits/container_test.go \
- ./e2e/common/traits/openapi_test.go \
- ./e2e/common/traits/service_test.go \
- -tags=integration -parallel=$(TEST_COMMON_PARALLEL_COUNT) $(TEST_INTEGRATION_COMMON_LANG_RUN) $(GOTESTFMT) || FAILED=1; \
- exit $${FAILED}
+ if [ $$FAILED -eq 0 ]; then \
Review Comment:
Is this condition skipping the test if the previous failed? I think that, as a smoke test, we want to test them all regardless each test is failing or not as this is mainly run for nightly release purposes.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] (fix#5315): concurrent map read and map write [camel-k]
Posted by "tdiesler (via GitHub)" <gi...@apache.org>.
tdiesler commented on code in PR #5375:
URL: https://github.com/apache/camel-k/pull/5375#discussion_r1568422882
##########
script/Makefile:
##########
@@ -267,19 +267,25 @@ test-common: do-build
test-smoke: do-build
FAILED=0; STAGING_RUNTIME_REPO="$(STAGING_RUNTIME_REPO)"; \
go test -timeout 10m -v ./e2e/common/main_test.go -tags=integration $(TEST_INTEGRATION_COMMON_LANG_RUN) $(GOTESTFMT) || FAILED=1; \
- go test -timeout 30m -v ./e2e/common/languages -tags=integration -parallel=$(TEST_COMMON_PARALLEL_COUNT) $(TEST_INTEGRATION_COMMON_LANG_RUN) $(GOTESTFMT) || FAILED=1; \
- go test -timeout 30m -v \
- ./e2e/common/misc/cron_test.go \
- ./e2e/common/misc/kamelet_test.go \
- ./e2e/common/misc/pipe_test.go \
- -tags=integration -parallel=$(TEST_COMMON_PARALLEL_COUNT) $(TEST_INTEGRATION_COMMON_LANG_RUN) $(GOTESTFMT) || FAILED=1; \
- go test -timeout 30m -v \
- ./e2e/common/traits/camel_test.go \
- ./e2e/common/traits/container_test.go \
- ./e2e/common/traits/openapi_test.go \
- ./e2e/common/traits/service_test.go \
- -tags=integration -parallel=$(TEST_COMMON_PARALLEL_COUNT) $(TEST_INTEGRATION_COMMON_LANG_RUN) $(GOTESTFMT) || FAILED=1; \
- exit $${FAILED}
+ if [ $$FAILED -eq 0 ]; then \
Review Comment:
You can now explicitly set
TEST_SKIP_AFTER_FAILURE_COUNT ?= 1
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org