Skip to content
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

Reset CDS Profile fetch counters + Introduce generic PeriodTaskExecutor #901

Merged
merged 14 commits into from
Apr 10, 2019

Conversation

dhaval24
Copy link
Contributor

@dhaval24 dhaval24 commented Apr 3, 2019

Currently CDS Profiler fetcher retries instantly 3 times. If the retry fails, it never retries for the entire application lifecycle. This leads to broken app-map experience as correlation needs ikey-appId resolution.

This PR fixes this by resetting the retry counter in CdsProfileFetcher.java every 4 hours by default. This configuration can be over-riden.

This PR also introduces a Generic Abstraction for creating, managing and running PeriodicTask called PeriodicTaskManager

In upcoming PR I will make this configurable via XML and Springboot starter.

@dhaval24 dhaval24 requested review from littleaj and reyang April 3, 2019 21:34
@dhaval24 dhaval24 self-assigned this Apr 3, 2019
Copy link
Contributor

@littleaj littleaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in my comments: The major things are the field visibility and log message.
Also, let's discuss the necessity of the resetService.
test assert suggestion is minor.

resetService = Executors.newSingleThreadScheduledExecutor(
ThreadPoolUtils.createDaemonThreadFactory(CdsProfileFetcher.class, "CdsProfilePurgeService"));
long cachePurgeInterval = policyConfiguration.getResetPeriodInMinutes();
resetService.scheduleAtFixedRate(new CachePurgingRunnable(), cachePurgeInterval, cachePurgeInterval,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhaval24 I wonder if we need an additional executor for purging the cache. If fetchProfile is used frequently, you could store the last time it was called and purge the cache after elapsed time is over the threshold.

Copy link
Contributor Author

@dhaval24 dhaval24 Apr 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@littleaj there is a slight confusion due to naming. This should more be resetInterval. I have pushed the changes. I do not purge cache and that is not needed. We only reset the counter. If cache part of InstrumentationKeyResolver already has the appId resolved there is no need to retry.

Regarding your comment on do we need additional service here. I would prefer to have seperate service as it adds clean seperation of what task is designated to which service.

@dhaval24 dhaval24 requested review from reyang and littleaj April 4, 2019 23:14
/**
* Cached instance to be reused across SDK for CDS Profile fetch calls.
*/
INSTANCE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good learning to me, never heard of it before :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the use of enum? :)

Copy link
Contributor Author

@dhaval24 dhaval24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In an offline conversation with @reyang he outlined additional concerns he has.

  1. Current design - has no ability to kill/stop a task. The util class should have it.
  2. The Map used to store taskId and service can be a source of memory leak as it stays for lifecycle of app.

To address @reyang comments. I will change the design to:

  1. Add cancellation capability for a task.
  2. Instead of creating a ExecutorService for each task, have a common service with 2 threads (can be tweaked in future as per need).
  3. Map stores string and future (which can be cancelled). Once a future is cancelled, it would be removed from the map.

@dhaval24 dhaval24 requested a review from reyang April 6, 2019 02:11
@dhaval24
Copy link
Contributor Author

dhaval24 commented Apr 6, 2019

@reyang I have redesigned the TimerTaskUtil and named it PeriodicTaskManager instead as it is mostly a management class and manages the lifecycle of PeriodicTask. Let me know your thoughts.

Additionally, I have also tried to resolve your previous comments.

@dhaval24 dhaval24 changed the title Reset CDS Profile fetch counters Reset CDS Profile fetch counters + Introduce generic PeriodTaskExecutor Apr 7, 2019
@dhaval24 dhaval24 requested a review from littleaj April 10, 2019 17:00
@dhaval24 dhaval24 merged commit e66ca6c into master Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants