I would propose to either increase TX_TIMEOUT or sleep multiplier to make test more reliable.
On Wed, Aug 9, 2017 at 3:32 PM, Николай Ижиков <[email protected]> wrote: > Vladimir, > > As far as I can understand behaviour of U.currentTimeMillis() breaks > transaction timeout test: > > https://issues.apache.org/jira/browse/IGNITE-5963 > > IgniteOptimisticTxSuspendResumeTest#testTxTimeoutOnSuspend : > > ``` > final Transaction tx = ignite.transactions().txStart(OPTIMISTIC, > isolation, > TX_TIMEOUT, 0); > > cache.put(1, 1); > > Thread.sleep(TX_TIMEOUT * 2); > > GridTestUtils.assertThrowsWithCause(new Callable<Object>() { > @Override public Object call() throws Exception { > tx.suspend(); > > return null; > } > }, TransactionTimeoutException.class); > ``` > Timeout checked like that: > > IgniteTxAdapter#remainingTime > > ``` > @Override public long remainingTime() { > if (timeout() <= 0) > return 0; > > long timeLeft = timeout() - (U.currentTimeMillis() - startTime()); > > return timeLeft <= 0 ? -1 : timeLeft; > > } > > ``` > > > 2017-08-09 15:26 GMT+03:00 Vladimir Ozerov <[email protected]>: > > > Guys, > > > > Are you really suggesting change the product for newcomer needs? This is > > not an argument for the change. Can someone explain me what is currently > > broken from product's perspective? Yes, we can get stale values for some > > time, we know that. Does it break something? > > > > On Wed, Aug 9, 2017 at 3:11 PM, Dmitry Pavlov <[email protected]> > > wrote: > > > > > +1 to Nikolay, this is very often question from newcomers. > > > It is not clear that current* method may return outdated value from > some > > > moment in past. > > > > > > Nikolay, how long outdated value can be returned by method? > > > > > > ср, 9 авг. 2017 г. в 15:00, Николай Ижиков <[email protected]>: > > > > > > > Vladimir, > > > > > > > > > There is nothing wrong with U.currentTimeMillis() at the > > > > moment. > > > > > > > > I think we can't rely on the return value for time measurement. > > > > Is it true? Is it OK for you? > > > > > > > > It very counterintuitive for me as newcomer. > > > > > > > > > > > > 2017-08-09 14:55 GMT+03:00 Vladimir Ozerov <[email protected]>: > > > > > > > > > You cannot check it with benchmarks, because behavior of this > method > > > will > > > > > vary between different JVMs, OSes and hardware. It can be different > > > even > > > > > with the same OS depending on it's settings. Again - let's just > avoid > > > > > unnecessary work. There is nothing wrong with U.currentTimeMillis() > > at > > > > the > > > > > moment. > > > > > > > > > > On Wed, Aug 9, 2017 at 2:52 PM, Dmitry Pavlov < > [email protected] > > > > > > > > wrote: > > > > > > > > > > > Vladimir, could we check it using benchmarks? Internet contains a > > lot > > > > of > > > > > > articles about this issue. But do we know if it is still actual > for > > > new > > > > > > VMs? > > > > > > > > > > > > ср, 9 авг. 2017 г. в 14:50, Dmitry Pavlov <[email protected] > >: > > > > > > > > > > > > > It seems System.currentTimeMillis () is now in intrinsic list. > > This > > > > > means > > > > > > > on modern JVMs performance penalty will not be so significiant. > > > > > > > > > > > > > > Nickolay, could you please raise standalone ticket for > > > > > > U.currentTimeMillis > > > > > > > () ? > > > > > > > > > > > > > > Could you also please check if system.nanoTime / > > > system.currentTimeMs > > > > > can > > > > > > > fix https://issues.apache.org/jira/browse/IGNITE-5963 When you > > > > create > > > > > a > > > > > > > PR, I can start several run for Ignite Cache 6 suite to check > if > > > > issue > > > > > is > > > > > > > still reprodacible. > > > > > > > > > > > > > > ср, 9 авг. 2017 г. в 14:41, Yakov Zhdanov <[email protected] > >: > > > > > > > > > > > > > >> Nickolay, IgniteUtils#currentTimeMillis() is some kind of an > > old > > > > > > heritage. > > > > > > >> I guess nobody remembers when this method has been > introduced. I > > > > agree > > > > > > >> that > > > > > > >> we can use System.currentTimeMillis(). I would suggest you > file > > a > > > > > ticket > > > > > > >> and replace this method calls with System.currentTimeMillis(). > > > > Sounds > > > > > > >> good? > > > > > > >> > > > > > > >> As far as reliable elapsed time measurement I agree with you > > that > > > > > > >> nanoTime() is better here, but it is definitely not a reason > for > > > > > > mentioned > > > > > > >> failure, since that test is launched in single JVM on a > machine > > > that > > > > > > most > > > > > > >> probably does not do any ntp syncs during the test to make > > > Ignite's > > > > > > >> timeout > > > > > > >> machinery fail. > > > > > > >> > > > > > > >> Please file a ticket to switch Ignite's timeouts to nanoTime() > > at > > > > some > > > > > > >> point. > > > > > > >> > > > > > > >> --Yakov > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > Nikolay Izhikov > > > > [email protected] > > > > > > > > > > > > > -- > Nikolay Izhikov > [email protected] >
