Feature/marker pagination fixes #298
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes it easier to debug issues that case 500 errors.
The implementation of AtomHopperJettyServerBuilder in test-suite did not have a constructor that takes config file location as argument, as the one in ah-jetty-server did. This change syncs the one in test-suite up with the one in ah-jetty-server.
MarkerRaceConditionTest posts multiple entries in parallel, while following the feed using the marker mechanism. The code is written to try to maximize the risk of having multiple entries being handled in parallel by the server. If any entry is written with an older timestamp than the latest, then that entry will be missed, and this will be revealed by the program.
When using a marker for pagination, entries were selected by comparing their dateLastUpdated with the creation date of the marker, which causes wrong results when using an updated marker.
This is required for high-resolution time stamps in Hibernate.
This, together with the hibernate-java8 module, enables use of high-resolution timestamps. In 5.2, the criteria support has been deprecated, and warnings are logged on each use of them. Migrating to a different mechanism is a large, separate task, so stay on 5.1 for now.
This reduces the probability of entries sharing the same timestamps. Shared timestamps cause issues with pagination.
If application servers have clocks that are not in sync, then it is possible for timestamps to not be increasing. If there are two application servers with out-of-sync clocks, where the clock of server A is a bit ahead of the clock of server B, then it's possible that an entry written by A gets a higher timestamp than en entry written at a later point by server B. If a consumer read the entry written by server A, and then used it as a marker, then it would not receive the entry written by server B. This change makes all application servers ask the database server for the clock, similar to how it is done in the Postgres adapter.
Otherwise, there's a race condition where newer entries can get older timestamps: if server A starts a transaction, gets a timestamp, but then gets blocked, and server B then starts a transaction, gets a higher timstamp, and commits, then when server A resumes, its entry will be written at a later time then the entry from server B, but with a lower timestamp. A consumer that read the entry from server B and used it as a marker would then not get the entry from server A. Locking the feed before getting the timestamp and holding it until the commit solves this issue, at the cost of serializing all writes to a feed.
Pagination doesn't work well when a marker has the same timestamp as other entries. If one producer writes an entry, a consumer reads it, and then another producer writes a new entry, and this gets the same timestamp as the first entry, then it's not clear whether the consumer should get this second entry or not when using the first entry as marker. If entries with the same timestamp as the marker should be included, then consumers may get entries multiple times if they share timestamp with the marker. If entries with the same timestamp as the marker should not be included, then consumers may miss entries. Using secondary sorting on ID also doesn't work, since it then is possible for a new entry that happen to get a lower-sorting ID to be considered older than an existing entry with a higher-sorting ID. This commit prevents these error conditions by forbidding sharing of timestamps between entries in the same feed. That way, a producer that happens to get the same timestamp as an earlier entry will have to retry, giving it a new timestamp. This ensures that consumers get each entry exactly once. Due to the high resolution time stamps and the lock on the feed, collisions should in practice never happen.
|
Can someone in the Cloud Feeds team review this patch? |
|
ok to test. |
|
thanks @reftel - we'll take a look at this. |
usnavi
reviewed
Oct 20, 2016
| } | ||
|
|
||
| /*This should throw the error because */ | ||
| @Test(expected=AtomDatabaseException.class) | ||
| @Test(expected=UnsupportedOperationException.class) | ||
| public void shouldThrowAtomDatabaseException() throws Exception { |
There was a problem hiding this comment.
Should we change the name of this test as its throwing a different exception?
| @@ -39,11 +41,8 @@ | |||
| @RunWith(Enclosed.class) | |||
| public class HibernateFeedRepositoryTest { | |||
|
|
|||
| public static class WhenPerformingSimpleAction { | |||
There was a problem hiding this comment.
Why are you removing the WhenPerformingSimpleAction and the WhenPerformingComplexAction test classes?
Should WhenCreatingMisconfiguredFeedRepository be an additional class rather than replacing?
| feedRepository.saveEntry(persistedEntry); | ||
|
|
||
| abderaParsedEntry.setUpdated(Date.from(persistedEntry.getDateLastUpdated())); | ||
| abderaParsedEntry.setPublished(Date.from(persistedEntry.getCreationDate())); | ||
|
|
There was a problem hiding this comment.
Why did you flip the order on this?
| import java.time.Instant; | ||
| import java.time.ZoneId; | ||
|
|
||
| public class NanoClock extends Clock |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This set of patches solves an issue where pagination using a marker would miss entries due to a race condition. The fixes are mostly for the Hibernate adapter, but the problem likely exists for other adapters as well. Whether other adapters have the same issue has not been tested, but the
MarkerRaceConditionTestshould be able to reproduce the error given a suitableapplication-context.xmland test environment.