Skip to content

[BEAM-8536] Migrate request_execution_time to request_delay_time in Java#10333

Merged
lukecwik merged 1 commit intoapache:masterfrom
boyuanzz:patch_again
Dec 17, 2019
Merged

[BEAM-8536] Migrate request_execution_time to request_delay_time in Java#10333
lukecwik merged 1 commit intoapache:masterfrom
boyuanzz:patch_again

Conversation

@boyuanzz
Copy link
Contributor

@boyuanzz boyuanzz commented Dec 9, 2019

@boyuanzz
Copy link
Contributor Author

Run Portable_Python PreCommit

@boyuanzz
Copy link
Contributor Author

Run Java PreCommit

1 similar comment
@boyuanzz
Copy link
Contributor Author

Run Java PreCommit

@boyuanzz
Copy link
Contributor Author

Kindly pinging : )

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there other places that need to be changed (e.g. does this need to be a two-phased commit)?

Copy link
Contributor Author

@boyuanzz boyuanzz Dec 16, 2019

Choose a reason for hiding this comment

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

I have changed all usages in this PR. Just talked to Rebo and Daniel, one thing left here is to generate go proto within this PR. I'll let you know once I finish.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have rebuilt the go proto. Waiting for tests pass.

@boyuanzz
Copy link
Contributor Author

Run Java PreCommit

@lukecwik
Copy link
Member

Run Python PreCommit

@lukecwik lukecwik merged commit 6021905 into apache:master Dec 17, 2019

// (Required) The application that should be scheduled.
BundleApplication application = 2;
BundleApplication application = 1;
Copy link
Member

Choose a reason for hiding this comment

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

Field indexes in protos should never be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comment! I think we should be fine here given:

  1. The usage of DelayedBundleApplication is still under development, we never announced an official supported usage so far IIRC.
  2. We release versioned SDK, which should also work with versioned runner. I don't think it will break anything.
    Would you mind explaining a little bit more about your concern?

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.

4 participants