Address intermittent failures of testQueryLoggingCount

[losipiuk]

The change addresses intermittent failures of AbstractTestDistributedQueries.testQueryLoggingCount.
The failure happened because of how beforeCompletedQueriesCount and
beforeSubmittedQueriesCount were determined at the beginning of tests.
Code assumed that it is safe to get those values as soon as all the
queries in queryManager are completed.

Yet it does not mean that stats counters were already updated, as those
two changes are not synchronized. See SqlQueryManager.createQuery:

Here is the code which updates stats counter for completed queries.

  QueryInfo info = queryExecution.getQueryInfo();
  stats.queryFinished(info);
  queryMonitor.queryCompletedEvent(info);

Yet according to info.isFinalQueryInfo() taken from
queryManager.getAllQueryInfo() the query is already completed.

fixes #8953

[losipiuk]

Dear reviewer: The effect of this change is very similar to just putting

sleepUninterruptibly(100, MILLISECONDS)

before this line.
I may change it, whichever you prefer.

[findepi]

minor comments

[findepi]

You could

lastValue = fun.get()

and then sleep before getting next value. This allow you to live without Optional

[findepi]

s/fun/valueSupplier/ or //computation

[losipiuk]

@findepi updated

[nezihyigitbasi]

waitUtilStable -> waitUntilStable

[nezihyigitbasi]

Since this method is only used for getting the before completed queries count, should we make it more specific like getBeforeCompletedQueriesCount? This is currently pretty generic and currently we don't need it to be.

I also feel like we should add a comment here that summarizes what's available in the git commit message. It's usually easier for the reader to just see it in the code rather than digging into the commit message.

[losipiuk]

I added comment. Not convinced to rename the method. It is fairly simple. And it's use in test is pretty self explanatory without need to look into body.
If something I would return it at all and replaced with sleep. See my comment #9409 (comment)

[nezihyigitbasi]

nit: Should we throw a TimeoutException here?

[losipiuk]

I used UncheckedTimeoutException from Guava.

[losipiuk]

@nezihyigitbasi, I addressed comments.

[nezihyigitbasi]

Thanks for fixing this, it has been bothering me for a while :) I just left a minor comment regarding the comment we added to the test.

[nezihyigitbasi]

This comment is confusing, because I understand from this comment that it's not safe to get the value of beforeSubmittedQueriesCount immediately after queries finish, however we don't use waitUntilStable for it below like we do for the other counter (in other words, if it's not safe why don't we wrap it with waitUntilStable). In fact, when I look at the code I think it's safe to get the number of submitted queries as that counter is updated synchronously (from createQuery() -> addStatsListeners() -- not in a final query info listener unlike the other counter).

How about we add this comment instead?

// We cannot simply get the number of completed queries as soon as all the queries are completed, because this counter may not be up-to-date at that point.
// The completed queries counter is updated in a final query info listener, which is called eventually.
// Therefore, here we wait until the value of this counter gets stable.

[losipiuk]

Oh good catch! I looked at just one counter and wrongly generalised how it is set it to both. Yet the reason why it is not wrapped with waitUntilStable is that I assumed that when one stabilises, then the other should be good too.

Anyway, whole logic is wrong for other counter so I will change the comment and commit message.