[GitHub] [ignite] sk0x50 commented on a change in pull request #8780: IGNITE-7641 Add IgniteCache#ttl method

[GitBox <gi...@apache.org>]

sk0x50 commented on a change in pull request #8780:
URL: https://github.com/apache/ignite/pull/8780#discussion_r619660321



##########
File path: modules/core/src/main/java/org/apache/ignite/IgniteCache.java
##########
@@ -1118,6 +1118,23 @@
      */
     public IgniteFuture<V> getAndReplaceAsync(K key, V val);
 
+    /**
+     * Returns the remaining time to live of the specified key.
+     *
+     * @param key Key
+     * @return remaining TTL in milliseconds, null if there is no expiry policy or EternalExpiryPolicy.

Review comment:
       Please specify the behavior of this method in the following cases:
    - the specified `key` as `null` 
    - cache does not contain the specified `key`
   
   perhaps, something like as follows:
   ```
      * @param key the key whose associated time-to-live value is to be returned
      * 
      * @return the remaining time-to-live, or {@code null}, if it does not exist or there is no expiry policy associated with the key or {@link EternalExpiryPolicy}.
      * @throws IllegalStateException if the cache is {@link #isClosed()}
      * @throws NullPointerException  if the key is null
   ```

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheAdapter.java
##########
@@ -3265,6 +3265,66 @@ protected V getAndRemove0(final K key) throws IgniteCheckedException {
         });
     }
 
+    /** {@inheritDoc} */
+    @Override public Long ttl(K key) {
+        boolean statsEnabled = ctx.statisticsEnabled();
+
+        long start = statsEnabled ? System.nanoTime() : 0L;
+
+        A.notNull(key, "key");
+
+        Long ttlVal = ttl0(key);
+
+        if (statsEnabled)
+            metrics0().addRemoveAndGetTimeNanos(System.nanoTime() - start);
+
+        return ttlVal;
+    }
+
+    /**
+     *
+     * @param key
+     * @return
+     */
+    protected Long ttl0(final K key) {
+        GridCacheEntryEx entry = entryEx(key);

Review comment:
       This approach does not seem correct to me. It will only work if the current node is an "affinity" node (it means that it hosts a primary or backup partition for the key).
   
   For instance, the following test fails:
   ```
       @Test
       public void testTtlMethodsFromClientNode() throws Exception {
           factory = CreatedExpiryPolicy.factoryOf(new Duration(TimeUnit.SECONDS, 100_000));
   
           disableEagerTtl = true;
   
           startGridsMultiThreaded(gridCount());
   
           IgniteEx client = startClientGrid(gridCount());
   
           IgniteCache c = client.cache(DEFAULT_CACHE_NAME);
   
           c.put(12, 12);
           
           assertEquals("Unexpected value!", c.get(12), 12);
           assertTrue("TTL equals to zero!", c.ttl(12) > 0);
       }
   ```
   
   I think the implementation should work in the way `IgniteCache#contains(K key)` works.




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