[GitHub] thrift pull request #1353: THRIFT-4327: add API to efficiently remove a sing...
Github user asfgit closed the pull request at: https://github.com/apache/thrift/pull/1353 ---
[GitHub] thrift pull request #1353: THRIFT-4327: add API to efficiently remove a sing...
Github user Typz commented on a diff in the pull request: https://github.com/apache/thrift/pull/1353#discussion_r138849538 --- Diff: lib/cpp/src/thrift/concurrency/TimerManager.h --- @@ -69,28 +72,33 @@ class TimerManager { * * @param task The task to execute * @param timeout Time in milliseconds to delay before executing task + * @return Handle of the timer, which can be used to remose the timer. --- End diff -- Done ---
[GitHub] thrift pull request #1353: THRIFT-4327: add API to efficiently remove a sing...
Github user Typz commented on a diff in the pull request: https://github.com/apache/thrift/pull/1353#discussion_r138849560 --- Diff: lib/cpp/test/concurrency/TimerManagerTests.h --- @@ -192,6 +192,38 @@ class TimerManagerTests { return true; } +/** + * This test creates two tasks, removes the first one then waits for the second one. It then + * verifies that the timer manager properly clean up itself and the remaining orphaned timeout + * task when the manager goes out of scope and its destructor is called. + */ + bool test03(int64_t timeout = 1000LL) { +TimerManager timerManager; +timerManager.threadFactory(shared_ptr(new PlatformThreadFactory())); +timerManager.start(); +assert(timerManager.state() == TimerManager::STARTED); + +Synchronized s(_monitor); + +// Setup the two tasks +shared_ptr taskToRemove += shared_ptr(new TimerManagerTests::Task(_monitor, timeout / 2)); +TimerManager::Timer timer = timerManager.add(taskToRemove, taskToRemove->_timeout); + +shared_ptr task + = shared_ptr(new TimerManagerTests::Task(_monitor, timeout)); +timerManager.add(task, task->_timeout); + +// Remove one task and wait until the other has completed +timerManager.remove(timer); --- End diff -- Done ---
[GitHub] thrift pull request #1353: THRIFT-4327: add API to efficiently remove a sing...
Github user Typz commented on a diff in the pull request: https://github.com/apache/thrift/pull/1353#discussion_r138845074 --- Diff: lib/cpp/src/thrift/concurrency/TimerManager.h --- @@ -100,13 +108,26 @@ class TimerManager { */ virtual void remove(stdcxx::shared_ptr task); + /** + * Removes a single pending task + * + * @param timer The timer to remove. The timer is returned when calling the + * add() method. + * @throws NoSuchTaskException Specified task doesn't exist. It was either + * processed already or this call was made for a + * task that was never added to this timer + * + * @throws UncancellableTaskException Specified task is already being + *executed or has completed execution. + */ + virtual void remove(Timer timer); --- End diff -- The old function removes *all* timers executing this task, and does this inefficiently by traversing the whole list. This new function removes a single timer in constant time [O(1)] which may be significant for large system with many timers. This does not impact the behavior of Thrift itself, since it does not rely on timer removal. ---
[GitHub] thrift pull request #1353: THRIFT-4327: add API to efficiently remove a sing...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1353#discussion_r138711396 --- Diff: lib/cpp/src/thrift/concurrency/TimerManager.h --- @@ -100,13 +108,26 @@ class TimerManager { */ virtual void remove(stdcxx::shared_ptr task); + /** + * Removes a single pending task + * + * @param timer The timer to remove. The timer is returned when calling the + * add() method. + * @throws NoSuchTaskException Specified task doesn't exist. It was either + * processed already or this call was made for a + * task that was never added to this timer + * + * @throws UncancellableTaskException Specified task is already being + *executed or has completed execution. + */ + virtual void remove(Timer timer); --- End diff -- Was removal of a task insufficient in some way? ---
[GitHub] thrift pull request #1353: THRIFT-4327: add API to efficiently remove a sing...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1353#discussion_r138711309 --- Diff: lib/cpp/test/concurrency/TimerManagerTests.h --- @@ -192,6 +192,38 @@ class TimerManagerTests { return true; } +/** + * This test creates two tasks, removes the first one then waits for the second one. It then + * verifies that the timer manager properly clean up itself and the remaining orphaned timeout + * task when the manager goes out of scope and its destructor is called. + */ + bool test03(int64_t timeout = 1000LL) { +TimerManager timerManager; +timerManager.threadFactory(shared_ptr(new PlatformThreadFactory())); +timerManager.start(); +assert(timerManager.state() == TimerManager::STARTED); + +Synchronized s(_monitor); + +// Setup the two tasks +shared_ptr taskToRemove += shared_ptr(new TimerManagerTests::Task(_monitor, timeout / 2)); +TimerManager::Timer timer = timerManager.add(taskToRemove, taskToRemove->_timeout); + +shared_ptr task + = shared_ptr(new TimerManagerTests::Task(_monitor, timeout)); +timerManager.add(task, task->_timeout); + +// Remove one task and wait until the other has completed +timerManager.remove(timer); --- End diff -- recommend calling remove a second time and validating expected behavior ---
[GitHub] thrift pull request #1353: THRIFT-4327: add API to efficiently remove a sing...
GitHub user Typz opened a pull request: https://github.com/apache/thrift/pull/1353 THRIFT-4327: add API to efficiently remove a single timer You can merge this pull request into a Git repository by running: $ git pull https://github.com/Typz/thrift THRIFT-4327 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1353.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1353 commit d1fd82a84af5346ce8b2c68cd8cb07c6533340a6 Author: Francois FerrandDate: 2017-09-11T10:09:40Z THRIFT-4327: add API to efficiently remove a single timer ---