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

JobTracker tracks all operations, not just jobs #2478

Open
sw96411 opened this issue Jul 16, 2021 · 1 comment · May be fixed by #3330
Open

JobTracker tracks all operations, not just jobs #2478

sw96411 opened this issue Jul 16, 2021 · 1 comment · May be fixed by #3330
Labels
bug Confirmed or suspected bug
Milestone

Comments

@sw96411
Copy link
Contributor

sw96411 commented Jul 16, 2021

The documentation of Job, and the general title/JavaDoc of JobTracker indicates that, when enabled on a store, the JobTracker should track only Jobs.

However, inspection of Store.java (see

addOrUpdateJobDetail(operation, context, null, JobStatus.RUNNING);
and
private JobDetail addOrUpdateJobDetail(final OperationChain<?> operationChain, final Context context, final String msg, final JobStatus jobStatus) {
) indicates that, when the JobTracker is enabled, any and every operation executed on the store is tracked as if it was a Job.

I suggest this is a defect, and that regular non-job operations should not be tracked by the JobTracker.

If this is, in fact, intended behaviour, then I suggest the documentation and Javadoc should be updated, and maybe the name of JobTracker changed to OperationTracker, as I'd argue the existing behaviour is surprising given the name of the class and documentation.

Impacts of this defect include:

  1. Performance degradation when running multiple fast operations caused by multiple writes to the cache for every operation
  2. Performance degradation when instantiating a Store (see the initialise method in Store.java), as initialising a store with the job tracker enabled requires iterating through everything tracked by the JobTracker (even jobs for other stores)
  3. In an environment where the JobTracker cache has a maximum size; early expiry of 'real' jobs as they are forced out of the cache early
  4. Data being persisted to the cache that the user might not expect

As always, happy to submit a PR (I think the solution to this issue is quite straightforward) if the maintainers agree that this is indeed a defect.

Thanks,

P.S. This issue has partially blocked the testing of gh-2457

GCHQDev404 added a commit that referenced this issue Jul 16, 2021
GCHQDev404 added a commit that referenced this issue Jul 16, 2021
GCHQDev404 added a commit that referenced this issue Jul 27, 2021
@t92549 t92549 added possible-bug question Specific query about part of the codebase labels Aug 3, 2021
@n3101 n3101 added bug Confirmed or suspected bug and removed possible-bug labels Aug 24, 2021
@n3101 n3101 added this to the v2_backlog milestone Aug 24, 2021
@n3101 n3101 removed the question Specific query about part of the codebase label Aug 30, 2022
@n3101
Copy link

n3101 commented Aug 30, 2022

We believe this is a bug.

On discussion with @GCHQDev404 it appears that this is no longer blocking completion of #2457 so it can stay on the post-2.0 backlog.

GCHQDev404 added a commit that referenced this issue Nov 2, 2022

* gh-2457-double-caching-issue weak initial step, requires synchronisation with FederatedGraphStorage and further testing.

* gh-2457-double-caching-issue remove FederatedGraphStorage local map, using cache only.

* gh-2457-double-caching-issue remove FederatedGraphStorage test fixes

* gh-2457-double-caching-issue remove FederatedGraphStorage review.

* gh-2457-double-caching-removing-graphstorage minimising use of GraphSerialisable.getGraph()

* gh-2457-double-caching-removing-graphstorage gh-2478 JobTracker cache can have Suffix name.

* gh-2457 double caching issue fix for persisting graph names in tests.

* Merge remote-tracking branch 'origin/v2-alpha' into gh-2357-federatedstore-federated-operation-merge-alpha2
!!!With 1 failing class of Tests!!!

* gh-2457 GraphSerialisable not being able to Mock has failing tests. changing GraphSerialisable causes backwards compatability issues.

* gh-2457 Fixed GraphSerialisable equals.

* gh-2457 checkstyle

* gh-2457 PR requests.

* gh-2457 PR requests.

* gh-2457 PR requests.
@ms9698 ms9698 linked a pull request Oct 31, 2024 that will close this issue
@tb06904 tb06904 linked a pull request Oct 31, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed or suspected bug
Projects
None yet
3 participants