[GitHub] thrift pull request #1353: THRIFT-4327: add API to efficiently remove a sing...

[asfgit]

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...

[Typz]

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...

[Typz]

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...

[Typz]

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...

[jeking3]

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...

[jeking3]

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...

[Typz]

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 Ferrand 
Date:   2017-09-11T10:09:40Z

THRIFT-4327: add API to efficiently remove a single timer




---