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

Fix Serialization Issue with 'isDefault' Field in DataFileRequest #262

Merged
merged 8 commits into from
Aug 14, 2023

Conversation

Zakaria-Kofiro
Copy link
Collaborator

@Zakaria-Kofiro Zakaria-Kofiro commented Aug 14, 2023

Fix Serialization Issue with 'isDefault' Field in DataFileRequest

When Tank jobs are run with only one DataFile, that DataFile in turn becomes the default DataFile for the job. This allows functions like #{ioFunctions.getCSVData(0)} to be used in scripts without the need to explicitly define which DataFile to pull from. This worked as intended until recently when it stopped setting single DataFiles as default, with the root cause of this issue being related to the serialization of the isDefault field. It was being serialized incorrectly to default and always had it's value set to it's default value of false when it was initially sent to the agent as true:

{"concurrentUsers":-1,"scriptUrl":"http://localhost:8080/tank/v2/jobs/script/150","rampTime":1500000,"jobId":"150","simulationTime":0,"startUsers":0,"userIntervalIncrement":2,"agentInstanceNum":0,"totalAgents":2,
"dataFiles":[{"fileName":"test.csv","fileUrl":"http://localhost:8080/tank/v2/datafiles/content?id=36&offset=0&lines=65","
default":false}]}

This was fixed by updating the field to isDefaultDataFile and setting the @JsonProperty and @XmlElement annotations accordingly:

{"concurrentUsers":-1,"scriptUrl":"http://localhost:8080/tank/v2/jobs/script/150","rampTime":1500000,"jobId":"150","simulationTime":0,"startUsers":0,"userIntervalIncrement":2,"agentInstanceNum":0,"totalAgents":2,
"dataFiles":[{"fileName":"test.csv","fileUrl":"http://localhost:8080/tank/v2/datafiles/content?id=36&offset=0&lines=65",
"isDefaultDataFile":true}]}

Please make sure these check boxes are checked before submitting

  • ** Squashed Commits **
  • ** All Tests Passed ** - mvn clean test -P default

** PR review process **

  • Requires one +1 from a reviewer
  • Repository owners will merge your PR once it is approved.

@@ -381,10 +381,11 @@ private void saveDataFile(DataFileRequest dataFileRequest) {
"writing file " + dataFileRequest.getFileName() + " to " + dataFile.getAbsolutePath()
+ " from url " + url.toExternalForm())));
FileUtils.copyURLToFile(url, dataFile);
if (dataFileRequest.isDefault()
if (dataFileRequest.isDefaultDataFile()
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Zakaria-Kofiro To unblock this change will resolve, but can we circle around with a follow-up ticket to rename the isDefaultDataFile property to something else?

Because this conditional upon reading is confusing because the conditional is saying:

  • IF it is a default data file flag TRUE, but the data file name is NOT equal the default file name csv-data.txt, then enter

It makes it seem like the two conditionals are contradicting, seems the isDefaultDataFile is actually is the number of data files equal to 1 based on https://github.com/intuit/Tank/blob/cefa8e5a720820970e1f49af1c3332d09d661cca/tank_vmManager/src/main/java/com/intuit/tank/perfManager/workLoads/JobManager.java#L302C27-L302C27

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, it confused me as well. The workflow is that it checks if there's a single datafile (in JobManager), and if there is, sets the conditional isDefaultDataFile to true from the controller. It is then sent to each agent which checks (in APITestHarness) whether both the default data flag is true and makes sure that it doesn't share a name with the default file name csv-data.txt. That's because it then copies the content from the original file to csv-data.txt which is then referenced throughout the rest of the code as the default datafile. I do agree that we should rename this, and maybe even revisit the workflow itself to just use the original file.

@shawn-h-park shawn-h-park self-requested a review August 14, 2023 18:49
@Zakaria-Kofiro Zakaria-Kofiro merged commit 3ceac01 into master Aug 14, 2023
@Zakaria-Kofiro Zakaria-Kofiro deleted the zkofiro/default-datafile-fix branch August 14, 2023 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants