Broken or Flaky test: PermissionsIT

[dlmarion]

PermissionsIT test methods are failing after merging #2707

[dlmarion]

This may not be related to #2707 after all, it may be related to the ZooKeeper property changes. The test sets some arbitrary table properties then confirms that the counts match. The test does not fail all the time. The stack trace is:

org.opentest4j.AssertionFailedError: expected: <0> but was: <1>
	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:166)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:161)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:628)
	at org.apache.accumulo.test.functional.PermissionsIT.testArbitraryProperty(PermissionsIT.java:877)
	at org.apache.accumulo.test.functional.PermissionsIT.testGrantedTablePermission(PermissionsIT.java:804)
	at org.apache.accumulo.test.functional.PermissionsIT.tablePermissionTest(PermissionsIT.java:641)
...

[ctubbsii]

I looked at several recently flaky tests... and they all seem to be the same type: do something that changes a property in ZK, then check that it was set to verify. I think what's happening is that the new prop store cache lookup is much faster now than with ZooCache previously, which looked up one property at a time. Because the cache lookup is faster, it's likely much faster than the watcher that triggers an update to the cache, and we're more likely to see stale values.

These tests could be improved by looping, waiting for the expected change (relying on timing out to fail the test if the asserted condition never happens, rather than an assert statement), or we can do something to force a sync on the server side request handling when users have a client request to view properties, so the client sees recent edits. I'd probably prefer the latter, so that way it improves the user experience, and we don't have to chase these tests one by one.

[dlmarion]

These tests could be improved by looping, waiting for the expected change (relying on timing out to fail the test if the asserted condition never happens, rather than an assert statement)

This could be made easier if we put the version number into the configuration. Then, the client could loop until a version of the properties come back with a version number greater than it had when it changed the configuration. For example:

long version = client.tableOperations().getProperties(tableName).get(Property.CONFIGURATION_VERSION.getKey());
// set iterator on table
while (client.tableOperations().getProperties(tableName).get(Property.CONFIGURATION_VERSION.getKey() <= version) {
  Thread.sleep(250);
}
// move on

[ctubbsii]

That's one way to do it, without any architecture changes. However, it really only helps for tests, and we'd have to modify each test to do this. There could still be a problem with collisions with this solution, and there are also still situations where you see that it is updated on the one server you asked... but then you perform an operation (like a scan), and the tserver doing that operation still sees the old version.

[EdColeman]

If the tests are setting multiple properties, it would help if the were bundled into an atomic set - that reduces the number of watch notifications that potentially need to be handled.

With the way ZooKeeper events are propagated clients receive notifications asynchronously - they will eventuality be consistent (baring additional changes)

Possible architectural changes for mitigation:

• send the properties with the scan request.
• send a version id of the properties to use (this might require keeping multiple, tagged versions so that a specified version is always available)
• send a timestamp that requests the properties have the same (or newer) timestamp. You could still end up with conflicts, but they would be more explicitly bound.

[ctubbsii]

If the tests are setting multiple properties, it would help if the were bundled into an atomic set - that reduces the number of watch notifications that potentially need to be handled.

That will be easier after #2717 is done

With the way ZooKeeper events are propagated clients receive notifications asynchronously - they will eventuality be consistent (baring additional changes)

Possible architectural changes for mitigation:

• send the properties with the scan request.
• send a version id of the properties to use (this might require keeping multiple, tagged versions so that a specified version is always available)

It might be nice to pass a generic metadata structure over every RPC call, so we can pass arbitrary state information to servers. We currently only send trace information that way, but we could expand that to include information about the caller's view of the configuration(s). The server could check to make sure that it is always at least as new as that version, and expire any cached items that are older, before proceeding. I wouldn't want to keep multiple versions, though. It should be fine as long as the server's view is at least as up-to-date as the client's. We don't need full isolated configuration... we just need to order client requests that use properties after processing any previous client requests to update properties.

We would also need to have the property setter RPC methods return a version once they are set, so the client knows what version they just set.

• send a timestamp that requests the properties have the same (or newer) timestamp. You could still end up with conflicts, but they would be more explicitly bound.

[dlmarion]

If I'm remembering the problem correctly from last week, the client modifies some properties and the server watcher has not fired before the client fetches the property to verify that it's been set. Have we considered having ZooPropStore.get(PropCacheKey) call ZooPropStore.readPropsFromZK(PropCacheKey) to confirm that the cached version matches the version in ZK?

[ctubbsii]

If I'm remembering the problem correctly from last week, the client modifies some properties and the server watcher has not fired before the client fetches the property to verify that it's been set. Have we considered having ZooPropStore.get(PropCacheKey) call ZooPropStore.readPropsFromZK(PropCacheKey) to confirm that the cached version matches the version in ZK?

That's basically equivalent to expiring the cache, which forces it to reload from ZK, which is what PR #2740 (and #2741) do.

[dlmarion]

That's basically equivalent to expiring the cache, which forces it to reload from ZK, which is what PR #2740 (and #2741) do.

I was just wondering if we should attempt to make the ZK clients (Accumulo server processes) more consistent. I know there is a cost to it, but if the server processes were more consistent then we would not need to invalidate the cache on a client call.