Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Fix update with large payload task result #2982

Merged
merged 2 commits into from
May 10, 2022

Conversation

apanicker-nflx
Copy link
Collaborator

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes (Please run ./gradlew generateLock saveLock to refresh dependencies)
  • WHOSUSING.md
  • Other (please describe):

Changes in this PR

Fixes a bug where large payload location in task result was not being propagated as part of task result by the client.

@apanicker-nflx apanicker-nflx added the type: bug bugs/ bug fixes label May 10, 2022
@apanicker-nflx apanicker-nflx changed the title fix update with large payload task result Fix update with large payload task result May 10, 2022
operation.apply(taskResult);
return taskResult;
Optional<String> optionalExternalStorageLocation = operation.apply(result);
if (optionalExternalStorageLocation.isPresent()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is confusing. Can we create two retryOperation method? one for the uploadtoexternalstorage, which takes a taskresult output map and returns an externalstoragepath, another one for the updatetask, which takes the final taskresult from last step, and return void

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to keep just one retryoperation method, we can just remove line #313 TaskResult taskResult = result.copy(); from it. I don't see the point of why we want to make a copy of it for every retry

@apanicker-nflx apanicker-nflx merged commit 8b1c50f into main May 10, 2022
@apanicker-nflx apanicker-nflx deleted the client_large_payload_fix branch May 10, 2022 22:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug bugs/ bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants