Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fetch specs from definitions directly #7293
fetch specs from definitions directly #7293
Changes from 14 commits
84837a1
728e828
3ec2217
cbe2b46
1e72752
b4438be
aa41cd4
9ec7bfd
4df74bc
545798b
928ae32
555b6fc
b48b46b
a23ba01
95e9f2f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
it looks like the only reason we need to keep this method are 2 usages.
ServerApp.java
line 222, which we know we are going to get rid of when we get rid of the file migrationsDockerImageValidator
- left a comment below for how we might be able to remove this usage.can we add a todo, reminding us that we want to kill this method?
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.
the reason i'm so focused on this method is because it has a different behavior that is non-obvious from the caller's point of view. the other methods all will try to find the spec in the definition itself. this one will never do that and will always start calling scheduler clients. so i want to advertise loudly that it is different and shouldn't be used. in fact, we should mark it as deprecated.
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.
how annoying! I see that usage in
SchedulerHandler
that is causing this. can you add a todo that says when we have moved the spec into the db as a required field we should get rid of the need for this?in case it's not clear, all jobs the require spinning up a docker container we use this
SynchronousResponse
struct to help pass through metadata around what happened in the job and logs. Once we can guarantee that for spec that we will not be spinning up a docker container, we can just remove this part from the code path.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.
As you pointed out, the main reason I added this was because the
SourceDefinitionSpecificationRead
struct returned by the SchedulerHandler method (which is in turned returned by the ConfigurationApi) contains a requiredjobInfo
field, so the method I added mocked out that job info when just getting the spec from the db.Therefore it seems to me that as part of the change to make the spec field required/guaranteed on the db struct, we should also remove the
jobInfo
field from theSourceDefinitionSpecificationRead
struct. Does this align with what you are suggesting here?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 actually think the code you've written here is good. i'm not suggesting we change anything. just want to add a comment saying that we want to get rid of this part of the public interface in the future.
you're right, we will want to remove
jobinfo
fromSourceDefinitionSpecificationRead
, but we should wait to do it until we have the guarantee that we will only ever fetch the spec from the db. the reality is that while it is still possible to pull specs from the docker image, it is still helpful to keep thejobinfo
because it includes logs to give clues as to what the problems are!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.
Completely agree! I meant that we should only remove
jobInfo
once we have that guarantee, and I agree that it makes sense to keep it for now 👍