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
CDAP-13867 fix to include program schedule while writing to RunMetaFi… #10440
Conversation
@@ -42,6 +47,14 @@ public ArtifactId getArtifactId() { | |||
return artifactId; | |||
} | |||
|
|||
/** | |||
* Get the program triggering schedule info as String, return null if its manually scheduled | |||
* @Nullable |
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.
this belongs outside of the javadoc
|
||
public ProgramStartInfo(Map<String, String> arguments, ArtifactId artifactId, String principal) { | ||
public ProgramStartInfo(Map<String, String> arguments, ArtifactId artifactId, String principal, | ||
@Nullable String scheduleInfo) { |
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's better to pass in the actual object here and return the actual object instead of a string. Though I guess you're not doing that because the serialization becomes really weird?
Maybe it would make sense to pass in all the system arguments then? I don't think storing the stringified schedule info is a good idea, it makes this code super brittle.
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.
yes, serialization becomes weird, and felt unnecessary, as its going to be written to file and not really used by the code that deserializes, however it might be better to store the entire system runtime arguments, as you mentioned to support future use cases if any, without breaking compat. ill make that change.
build running https://builds.cask.co/browse/CDAP-RUT1595-2 |
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.
style comment, othewise lgtm. fix and merge
Schema.Field.of(Constants.ARTIFACT_ID, ARTIFACT_INFO)); | ||
Schema.Field.of(Constants.ARTIFACT_ID, ARTIFACT_INFO), | ||
Schema.Field.of(Constants.SYSTEM_ARGUMENTS, Schema.mapOf(Schema.of(Schema.Type.STRING), | ||
Schema.of(Schema.Type.STRING)) |
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.
indentation
…leSet and reading it correspondingly in spark app
a16c21f
to
1a977db
Compare
build green - https://builds.cask.co/browse/CDAP-RUT1595-4 merging |
…leSet and reading it correspondingly in spark app
Summary
Report Generation Spark job, assumes that the triggeringScheduleInfo necessary to figure out the startMethod is available as part of runtime arguments.
However this is user runtime arguments and the triggeringScheduleInfo is present in system arguments.
Due to this missing information the startMethod will always be "manual" even for scheduled runs.
added triggeringScheduleInfo to program start info and made appropriate change to read this triggeringScheduleInfo from the programStartInfo rather than runtime arguments in the spark job.
The test case has coverage for scheduled runs, however it passes that information through runtime arguments and the code path in unit test doesn't cover reading from TMS and writing to RunMetaFileset.