Timing out stale remote master history by masseyke · Pull Request #86936 · elastic/elasticsearch

masseyke · 2022-05-19T17:56:02Z

This change makes it so that the remote master history that has been fetched will time out after 5 minutes (by default). The MasterHistoryService will return its value as null if it is older than the configured timeout. This way MasterHistoryService clients don't report status based on out of date information.

elasticmachine · 2022-05-19T18:56:34Z

Pinging @elastic/es-data-management (Team:Data Management)

andreidan

Thanks for working on this Keith.

This generally LGTM - just left a few questions

andreidan · 2022-05-20T07:54:58Z

server/src/main/java/org/elasticsearch/cluster/coordination/MasterHistoryService.java

+     */
+    public static final Setting<TimeValue> MAX_USABLE_REMOTE_HISTORY_AGE_SETTING = Setting.timeSetting(
+        "master_history.max_usable_remote_history_age",
+        DEFAULT_MAX_USABLE_REMOTE_HISTORY_AGE,


Should this mandate a min-value?

What do you think it ought to be? I thought briefly about it, and then thought maybe there would be some case where we would want to tell a user to set it to 0 or negative to basically skip checking remote master history (probably not, but we might be kicking ourselves later if we put in some arbitrary minimum).

Shall we use positiveTimeSetting to at least not worry about negative values?

andreidan · 2022-05-20T07:57:14Z

server/src/main/java/org/elasticsearch/cluster/coordination/MasterHistoryService.java

+    volatile RemoteHistoryOrException remoteHistoryOrException = new RemoteHistoryOrException(null, null, Long.MIN_VALUE);
    private static final Logger logger = LogManager.getLogger(MasterHistoryService.class);

+    private static final TimeValue DEFAULT_MAX_USABLE_REMOTE_HISTORY_AGE = new TimeValue(5, TimeUnit.MINUTES);


Is "usable" a bit redundant in the name? Would going with "time to live" be a bit more intuitive? (given the already established understanding in caching technologies) e.g. "remote_history_time_to_live" ?

andreidan · 2022-05-20T08:01:22Z

server/src/main/java/org/elasticsearch/cluster/coordination/MasterHistoryService.java

+         */
+        long acceptableRemoteHistoryTime = currentTimeMillisSupplier.getAsLong() - acceptableRemoteHistoryAge.getMillis();
+        if (remoteHistoryOrExceptionCopy.creationTimestamp < acceptableRemoteHistoryTime) {
+            return null;


Would we want to also at some point null-ify remoteHistoryOrException ? (we'd likely need an AtomicReference or such to do a compare and swap)

What do you think?

My thinking was that there was no harm in holding onto a single one, and it avoids having to synchronize access to it. Synchronizing it would not be super expensive (we'd need to read it, do the time calculation, and set it to null in an atomic action), but it seemed like an unnecessary complication. Is it the memory that you're concerned about? We cap these things to have at most 50 entries, so I'd expect at most a few KB here.

I was mostly worried about the remoteHistoryOrException leaking outside the service by other means than getRemoteMasterHistory or the service evolving and using the remoteHistoryOrException in a private way (assuming it's null when it's stale, which would not be a wild assumption given what the getter returns) - @DaveCTurner would you have a strong opinion here?

That second concern would maybe mean having a thread to time this thing out. That seems like something we ought to deal with if we really need it in the future, but seems overly complex for the current situation doesn't it?

I am inclined to leave it in place too. You never know, it might even end up being useful in a heap dump. Maybe add a comment to the field indicating that it might be stale.

For future reference it wouldn't be a big deal to clear it after the timeout either:

transportService.getThreadPool().schedule(() -> {/* clear field*/}, acceptableRemoteHistoryAge, Names.SAME);

(also there's no need for a ThreadPool constructor argument if you have a TransportService)

++ let's leave it as is then

masseyke · 2022-05-20T14:57:19Z

@elasticmachine run elasticsearch-ci/part-1

andreidan · 2022-05-24T09:23:52Z

server/src/main/java/org/elasticsearch/cluster/coordination/MasterHistoryService.java


-    record RemoteHistoryOrException(List<DiscoveryNode> remoteHistory, Exception exception) { // non-private for testing
+    // non-private for testing
+    record RemoteHistoryOrException(List<DiscoveryNode> remoteHistory, Exception exception, long creationTimestamp) {


Suggested change

record RemoteHistoryOrException(List<DiscoveryNode> remoteHistory, Exception exception, long creationTimestamp) {

record RemoteHistoryOrException(List<DiscoveryNode> remoteHistory, Exception exception, long creationTimeMillis) {

andreidan

LGTM, with a minor rename suggestion

Thanks Keith

andreidan · 2022-05-24T09:24:04Z

server/src/main/java/org/elasticsearch/cluster/coordination/MasterHistoryService.java


-        RemoteHistoryOrException(List<DiscoveryNode> remoteHistory) {
-            this(remoteHistory, null);
+        RemoteHistoryOrException(List<DiscoveryNode> remoteHistory, long creationTimestamp) {


Can we rename the creationTimestamp to include the unit everywhere? ( https://github.com/elastic/elasticsearch/pull/86936/files#r880273628 )

masseyke · 2022-05-24T20:11:42Z

@elasticmachine run elasticsearch-ci/part-1

masseyke · 2022-05-24T20:12:23Z

@elasticmachine run elasticsearch-ci/part-2

Timing out stale remote master history

7c758c4

masseyke added >non-issue :Distributed/Health Issues for the health report API v8.3.0 labels May 19, 2022

masseyke added 2 commits May 19, 2022 13:53

Adding to javadocs

649f5fa

Adding to javadocs

caf530a

masseyke marked this pull request as ready for review May 19, 2022 18:56

elasticmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label May 19, 2022

masseyke requested a review from andreidan May 19, 2022 18:56

masseyke mentioned this pull request May 19, 2022

Master stability health indicator part 1 (when a master has been seen recently) #86524

Merged

andreidan reviewed May 20, 2022

View reviewed changes

renaming setting

995c4b0

masseyke added 3 commits May 23, 2022 10:34

code review feedback

a103db2

Merge branch 'master' into fix/age-out-remote-master-history

3cd4b3d

Adding a comment about remote history TTL

8327203

masseyke requested a review from andreidan May 23, 2022 18:15

andreidan reviewed May 24, 2022

View reviewed changes

andreidan approved these changes May 24, 2022

View reviewed changes

code review feedback

5107b18

Merge branch 'master' into fix/age-out-remote-master-history

e6fa99d

masseyke merged commit 3630a14 into elastic:master May 24, 2022

masseyke deleted the fix/age-out-remote-master-history branch May 24, 2022 21:16

	record RemoteHistoryOrException(List<DiscoveryNode> remoteHistory, Exception exception, long creationTimestamp) {
	record RemoteHistoryOrException(List<DiscoveryNode> remoteHistory, Exception exception, long creationTimeMillis) {

Conversation

masseyke commented May 19, 2022

Uh oh!

elasticmachine commented May 19, 2022

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masseyke commented May 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreidan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masseyke commented May 24, 2022

Uh oh!

masseyke commented May 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants