-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor lookups behavior while loading/dropping the containers #14806
Changes from all commits
383eb3d
913768a
57d2a19
e6b1ce1
7a8a26b
b30022f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,4 +49,9 @@ default long getJitterMills() | |
{ | ||
return 0; | ||
} | ||
|
||
default long getLoadTimeoutMills() | ||
{ | ||
return 60 * 1000; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. QQ. Is there any reason we have kept this to be 60 sec but the one for default on Jdbc extraction namespace is 120 sec ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, default loadTime for lookups is 60 secs that will be enough time to load all non jdbc lookups like uri, maps, and other loaders. Overriding this loadTime for JDBC to 2 mins as it can take little longer to load entries if they in millions. Also documented it so that users can customize it for their use cases of big lookups. |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would updating the topic or some other config of the kafka lookup also result in a new cache being initialized, but because we just return true here we potentially don't wait on it? I guess i'm wanting to make sure that it isn't a problem that this one wouldn't wait until it has read the topic and populated the cache for the first time, or is it not a problem for some other reason (i tried to remember how all this stuff works, but i'm still not certain 🙃 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For first time, KafkaLookupExtractorFactory has lifecycle and process will wait for started latch in start method.
Today, On updating the config, it deletes the old lookup immediately and it kicks the start lifecycle again. I just wanted to keep the same behavior as this change was mainly to cover jdbc use case, thus overridden await methods.
But you have great point, I think we should await for kafka, reading from kafka and populating is asynchronus and ideally it should await on initialization as well to avoid the disruption in the looking serving requests.
New behavior will be:
kafka lookup will wait until forever (as there is no definite loadTimeout that we have currently for kafka lookups) to drain all messages from kafka topic and it will be initialized fully and then it will drop the old container and starting serving from new one, else it will continue serving from old lookup.
Question is: should we do this change along with same PR or not. I would prefer to do this in next followup PR as it needs bunch of unit tests case to cover the the wait scenario until topic is drained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think its fine to do as a follow-up, thanks for looking into it