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

2478: JobTracker tracks all operations not just jobs #3330

Closed

Conversation

ms9698
Copy link
Member

@ms9698 ms9698 commented Oct 31, 2024

Changes Made:
Removed addOrUpdateJobDetail from the execute method.
Non-job operations will no longer be tracked, preventing unnecessary performance degradation and cache pollution.
Removed test for checking if operation runs when jobtracker cache is broken.

@tb06904 tb06904 linked an issue Oct 31, 2024 that may be closed by this pull request
@tb06904 tb06904 added this to the 2.3.1 milestone Oct 31, 2024
@tb06904 tb06904 added the bug Confirmed or suspected bug label Oct 31, 2024
@tb06904 tb06904 modified the milestones: 2.3.1, 2.4.0 Nov 1, 2024
@ms9698 ms9698 force-pushed the 2478-jobtracker-tracks-all-operations-not-just-jobs branch from 93a3a3d to 906e3fa Compare November 13, 2024 09:39
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 68.01%. Comparing base (960c599) to head (d7ebdd9).

Files with missing lines Patch % Lines
.../src/main/java/uk/gov/gchq/gaffer/store/Store.java 40.00% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             develop    #3330   +/-   ##
==========================================
  Coverage      68.01%   68.01%           
  Complexity      2596     2596           
==========================================
  Files            955      955           
  Lines          30564    30559    -5     
  Branches        3376     3376           
==========================================
- Hits           20787    20784    -3     
+ Misses          8302     8300    -2     
  Partials        1475     1475           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@p29876
Copy link
Member

p29876 commented Nov 19, 2024

Can you fix the code smell?

store.initialise("graphId", createSchemaMock(), storeProperties);

// When
store.execute(addElements, context);
Copy link
Member

Choose a reason for hiding this comment

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

This test is worth keeping as it tests job functionality
We no longer handle jobs in the execute method, but we do in the executeJob method.
Swapping this line should pass the test

@ms9698 ms9698 force-pushed the 2478-jobtracker-tracks-all-operations-not-just-jobs branch from 0cc7bef to f85d483 Compare November 19, 2024 11:27
@@ -106,24 +106,6 @@ public void shouldPropagateStatusInformationContainedInOperationExceptionsThrown
assertEquals(SERVICE_UNAVAILABLE.getStatusCode(), response.getStatus());
}

@Test
public void shouldReturnSameJobIdInHeaderAsGetAllJobDetailsOperation() throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

Agree that this test can be deleted now as it fails due to no jobs running which is correct.
However, I'm not a fan of just deleting tests with no explanation. It's better to change the test to document the new behavior as a bit of self documenting code. I think we need a new test here to show that we shouldn't get a job-id header returned anymore for the executeOperation endpoint as it doesn't make sense.

However, we are still getting the header returned so you will need to implement this

Copy link
Member

@p29876 p29876 left a comment

Choose a reason for hiding this comment

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

Can we stop the execute endpoint from returning a job-id header as it no longer makes sense to return one?

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
Development

Successfully merging this pull request may close these issues.

JobTracker tracks all operations, not just jobs
3 participants