You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celix.apache.org by GitBox <gi...@apache.org> on 2020/07/09 11:47:27 UTC
[GitHub] [celix] Oipo opened a new pull request #267: Make pstm handling thread sleep time configurable
Oipo opened a new pull request #267:
URL: https://github.com/apache/celix/pull/267
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] abroekhuis commented on a change in pull request #267: Make pstm handling thread sleep time configurable
Posted by GitBox <gi...@apache.org>.
abroekhuis commented on a change in pull request #267:
URL: https://github.com/apache/celix/pull/267#discussion_r452202816
##########
File path: bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.c
##########
@@ -79,6 +79,7 @@ celix_status_t pubsub_topologyManager_create(celix_bundle_context_t *context, ce
manager->loghelper = logHelper;
manager->verbose = celix_bundleContext_getPropertyAsBool(context, PUBSUB_TOPOLOGY_MANAGER_VERBOSE_KEY, PUBSUB_TOPOLOGY_MANAGER_DEFAULT_VERBOSE);
+ manager->handlingThreadSleepTime = celix_bundleContext_getPropertyAsLong(context, PUBSUB_TOPOLOGY_MANAGER_HANDLING_THREAD_SLEEPTIME_SECONDS_KEY, PSTM_PSA_HANDLING_DEFAULT_SLEEPTIME_IN_SECONDS);
Review comment:
I'm fine with this solution, if it ever comes up again, we can always take a look at potential risks. I doubt anyone is going to change the timeout at runtime.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] Oipo commented on a change in pull request #267: Make pstm handling thread sleep time configurable
Posted by GitBox <gi...@apache.org>.
Oipo commented on a change in pull request #267:
URL: https://github.com/apache/celix/pull/267#discussion_r452161724
##########
File path: bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.c
##########
@@ -79,6 +79,7 @@ celix_status_t pubsub_topologyManager_create(celix_bundle_context_t *context, ce
manager->loghelper = logHelper;
manager->verbose = celix_bundleContext_getPropertyAsBool(context, PUBSUB_TOPOLOGY_MANAGER_VERBOSE_KEY, PUBSUB_TOPOLOGY_MANAGER_DEFAULT_VERBOSE);
+ manager->handlingThreadSleepTime = celix_bundleContext_getPropertyAsLong(context, PUBSUB_TOPOLOGY_MANAGER_HANDLING_THREAD_SLEEPTIME_SECONDS_KEY, PSTM_PSA_HANDLING_DEFAULT_SLEEPTIME_IN_SECONDS);
Review comment:
No specific reason, no. Just another case of ctrl+C, ctrl+V.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] Oipo commented on a change in pull request #267: Make pstm handling thread sleep time configurable
Posted by GitBox <gi...@apache.org>.
Oipo commented on a change in pull request #267:
URL: https://github.com/apache/celix/pull/267#discussion_r452162377
##########
File path: bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.c
##########
@@ -79,6 +79,7 @@ celix_status_t pubsub_topologyManager_create(celix_bundle_context_t *context, ce
manager->loghelper = logHelper;
manager->verbose = celix_bundleContext_getPropertyAsBool(context, PUBSUB_TOPOLOGY_MANAGER_VERBOSE_KEY, PUBSUB_TOPOLOGY_MANAGER_DEFAULT_VERBOSE);
+ manager->handlingThreadSleepTime = celix_bundleContext_getPropertyAsLong(context, PUBSUB_TOPOLOGY_MANAGER_HANDLING_THREAD_SLEEPTIME_SECONDS_KEY, PSTM_PSA_HANDLING_DEFAULT_SLEEPTIME_IN_SECONDS);
Review comment:
I don't think the overhead of calling `celix_bundleContext_getPropertyAsLong()` matters much, but that's the only reason I can think of not wanting to do it inside of the loop.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] Oipo commented on a change in pull request #267: Make pstm handling thread sleep time configurable
Posted by GitBox <gi...@apache.org>.
Oipo commented on a change in pull request #267:
URL: https://github.com/apache/celix/pull/267#discussion_r452163867
##########
File path: bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.c
##########
@@ -79,6 +79,7 @@ celix_status_t pubsub_topologyManager_create(celix_bundle_context_t *context, ce
manager->loghelper = logHelper;
manager->verbose = celix_bundleContext_getPropertyAsBool(context, PUBSUB_TOPOLOGY_MANAGER_VERBOSE_KEY, PUBSUB_TOPOLOGY_MANAGER_DEFAULT_VERBOSE);
+ manager->handlingThreadSleepTime = celix_bundleContext_getPropertyAsLong(context, PUBSUB_TOPOLOGY_MANAGER_HANDLING_THREAD_SLEEPTIME_SECONDS_KEY, PSTM_PSA_HANDLING_DEFAULT_SLEEPTIME_IN_SECONDS);
Review comment:
Maybe thread-safety? Hrm.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] Oipo commented on a change in pull request #267: Make pstm handling thread sleep time configurable
Posted by GitBox <gi...@apache.org>.
Oipo commented on a change in pull request #267:
URL: https://github.com/apache/celix/pull/267#discussion_r452161308
##########
File path: bundles/pubsub/test/CMakeLists.txt
##########
@@ -173,12 +173,6 @@ if (BUILD_PUBSUB_PSA_UDP_MC)
add_test(NAME pstm_deadlock_udpmc_test COMMAND pstm_deadlock_udpmc_test WORKING_DIRECTORY $<TARGET_PROPERTY:pstm_deadlock_udpmc_test,CONTAINER_LOC>)
setup_target_for_coverage(pstm_deadlock_udpmc_test SCAN_DIR ..)
- #TCP Endpoint test is disabled because the test is not stable when running on Travis
Review comment:
Yes. If you look closely, it's trying to add a TCP test inside of the UDP block. The TCP block also adds this test, it must've somehow got duplicated.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes merged pull request #267: Make pstm handling thread sleep time configurable
Posted by GitBox <gi...@apache.org>.
pnoltes merged pull request #267:
URL: https://github.com/apache/celix/pull/267
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] abroekhuis commented on a change in pull request #267: Make pstm handling thread sleep time configurable
Posted by GitBox <gi...@apache.org>.
abroekhuis commented on a change in pull request #267:
URL: https://github.com/apache/celix/pull/267#discussion_r452159799
##########
File path: bundles/pubsub/test/CMakeLists.txt
##########
@@ -173,12 +173,6 @@ if (BUILD_PUBSUB_PSA_UDP_MC)
add_test(NAME pstm_deadlock_udpmc_test COMMAND pstm_deadlock_udpmc_test WORKING_DIRECTORY $<TARGET_PROPERTY:pstm_deadlock_udpmc_test,CONTAINER_LOC>)
setup_target_for_coverage(pstm_deadlock_udpmc_test SCAN_DIR ..)
- #TCP Endpoint test is disabled because the test is not stable when running on Travis
Review comment:
Was this removed intentionally?
##########
File path: bundles/pubsub/pubsub_topology_manager/src/pubsub_topology_manager.c
##########
@@ -79,6 +79,7 @@ celix_status_t pubsub_topologyManager_create(celix_bundle_context_t *context, ce
manager->loghelper = logHelper;
manager->verbose = celix_bundleContext_getPropertyAsBool(context, PUBSUB_TOPOLOGY_MANAGER_VERBOSE_KEY, PUBSUB_TOPOLOGY_MANAGER_DEFAULT_VERBOSE);
+ manager->handlingThreadSleepTime = celix_bundleContext_getPropertyAsLong(context, PUBSUB_TOPOLOGY_MANAGER_HANDLING_THREAD_SLEEPTIME_SECONDS_KEY, PSTM_PSA_HANDLING_DEFAULT_SLEEPTIME_IN_SECONDS);
Review comment:
Just wondering, doing this in the create makes it impossible to update the timeout when the component is active.
Getting it where needed, makes this possible. (I doubt this use case is needed..)
Any specific reasons to do it here and not in the handler thread?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org