Skip to content

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

Merged
Zakaria-Kofiro merged 8 commits intomasterfrom
zkofiro/default-datafile-fix
Aug 14, 2023
Merged

Fix Serialization Issue with 'isDefault' Field in DataFileRequest#262
Zakaria-Kofiro merged 8 commits intomasterfrom
zkofiro/default-datafile-fix

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.

+ " from url " + url.toExternalForm())));
FileUtils.copyURLToFile(url, dataFile);
if (dataFileRequest.isDefault()
if (dataFileRequest.isDefaultDataFile()
Copy link
Contributor

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

@Zakaria-Kofiro Zakaria-Kofiro Aug 14, 2023

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.

2 participants