Add delay for system test.

[stephenplusplus]

Fixes #190

Some of our tests query table data immediately after it is inserted. Locally, this seems to work fine, however on CI (update: still failing), our queries return stale data.

This change will add a 500ms delay between data insertion and query.

[codecov]

Codecov Report

Merging #16 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #16   +/-   ##
=======================================
  Coverage   99.53%   99.53%           
=======================================
  Files          12       12           
  Lines        1281     1281           
=======================================
  Hits         1275     1275           
  Misses          6        6

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be57ff7...53d5eaf. Read the comment docs.

[vkedia]

There should not be any need to sleep. As soon as insert returns you should be able to read the latest data if you are using strong reads.

[stephenplusplus]

I've re-opened this, because I can't think of another solution. We're still seeing this flakiness-- the query sometimes returns the row inserted in the previous test.

[alexander-fenster]

I feel bad about adding timeouts to tests. They will hide real problems and will not actually make the test more stable.

Can we make this change in a branch (not in a forked repo), then remove the branch filter from CircleCI config in this branch to make it run system tests, and make several CI runs, I would say 10, to verify there are no flaky errors? If it really helps, get back to @vkedia to understand the reason.

[stephenplusplus]

@vkedia is this a strong read? If not, your comment leads me to believe that a latency is expected between data insertion and querying availability. But, what I don't know, is if this is a strong read or eventual.

[stephenplusplus]

I feel bad about adding timeouts to tests. They will hide real problems and will not actually make the test more stable.

I don't mind a timeout to give the server the time it needs to handle the operation. That's unavoidable. An alternative approach is polling, but if 5 seconds is always long enough, then polling would just add complication to the tests.

In either case, I'd like @vkedia's input first.

[alexander-fenster]

Friendly ping on this - system tests are still flaky and it makes me so sad :(

[stephenplusplus]

cc @snehashah16

[snehashah16]

I agree with vkedia's input, there is no reason to add this sleep.
Setting Strong in BeginTxn here

I am Node - illiterate, and therefore can provide a java example so that this works:

DatabaseClient client = Spanner.... 
// Strong bound is default. Though u can explicitly put it in your test to verify functionality
ReadOnlyTransaction txn = client.singleUseReadOnlyTransaction(); 
ResultSet resultSet = txn.read(table, KeySet.singleKey(key), columns);
assertThat(txn.getReadTimestamp()).isAtLeast(expected.timestamp);

[snehashah16]

@stephenplusplus - if this does not work, I assure u its an issue in the Node library code, and please dig into the root cause.
This has been extensively tested on Spanner and should work as intended.

[stephenplusplus]

Thanks @snehashah16. The tests were previously not running as part of a transaction. I switched to using transactions, but I'm not sure if that's what we want here (@callmehiphop ?).

[callmehiphop]

@stephenplusplus I could be wrong, but I think that any calls made by a Table object get ran as a single-use transaction, so I don't think we need to explicitly run that as a transaction?

[stephenplusplus]

@callmehiphop Ah, thanks for the tip. Now, I just pass the transaction options to the database.run/runStream call directly. That seems like it will still enforce the transaction as a strong read.

[snehashah16]

@callmehiphop - could u please double check, single-use txn should use Strong Bound by default.

@stephenplusplus -
this should also work with singleUse, so please add appropriate tests.

[stephenplusplus]

Here's the code for database.runStream:

Database.prototype.runStream = function(query, options) {
  var self = this;

  if (is.string(query)) {
    query = {
      sql: query,
    };
  }

  var reqOpts = codec.encodeQuery(query);

  if (options) {
    reqOpts.transaction = {
      singleUse: {
        readOnly: TransactionRequest.formatTimestampOptions_(options),
      },
    };
  }

  delete reqOpts.json;
  delete reqOpts.jsonOptions;

  function makeRequest(resumeToken) {
    return self.pool_.requestStream({
      client: 'SpannerClient',
      method: 'executeStreamingSql',
      reqOpts: extend(reqOpts, {resumeToken}),
    });
  }

  return new PartialResultStream(makeRequest, {
    json: query.json,
    jsonOptions: query.jsonOptions,
  });
};

It looks like it's only a single use transaction if options are passed? And strong is not set by default, but it sounds like we need to do that?

[callmehiphop]

@stephenplusplus I believe that is correct

[stephenplusplus]

@snehashah16 outside of a transaction, should we always use a single-use transaction? And if so, should we default to strong? Any other comments about how our default behavior should work are welcome.

[stephenplusplus]

ping @snehashah16 about the question above. Our method, database.run('SELECT ...') by default, does not create a transaction (single-use or otherwise). A user can choose to run in a single-use transaction by passing an options object to the method. Should we opt the user into a single-use transaction by default? If so, should we set strong reads by default?

I believe the fact that our code was not doing this contributed to why the delay proposed in this PR was required to receive accurate query results.

[snehashah16]

@stephenplusplus - Java always has strong reads for singleUse()

public ReadContext singleUse() { return singleUse(TimestampBound.strong()); }

Should we opt the user into a single-use transaction by default?

Yes, my recommendation

[jkwlui]

Can we merge here?

[JustinBeckwith]

@stephenplusplus let me know if I shouldn't have done that 🤣