Skip to content

Re revert go sdk#4818

Closed
kennknowles wants to merge 297 commits intoapache:go-sdkfrom
kennknowles:re-revert-go-sdk
Closed

Re revert go sdk#4818
kennknowles wants to merge 297 commits intoapache:go-sdkfrom
kennknowles:re-revert-go-sdk

Conversation

@kennknowles
Copy link
Member

Into the Go SDK branch, merges master up to revert of accidental Go SDK, plus re-revert to put the Go SDK back.

apilloud and others added 30 commits February 15, 2018 10:31
Also close the reader at end of reader.
Updated the test for reader reuse.
[BEAM-3552] Support Python 3 in the metrics, internal, typehints, and utils modules.
…imers for the DirectRunner.

[BEAM-3153] Add processing-time timers for the DirectRunner.
to use of `extractOrderedList()` which removes all the elements from
the heap. `extractOrderedList()` is costly and is not required either.
`extractOutput()` does not mutate now and is cheaper too.

Updated LargestUnique.add() to avoid 'heap.contains()' call in common
case with large input.

Merging was not tested as direct-runner does not seem to use combiner.
Added test using ConbineFnTester.

Put back add() improvement. contains() is an O(n) operation.
Avoid it in common case. I think the extractOrderedList() existed mainly
to avoid this.
…unner

[BEAM-3689] Fix unbounded reader leak in direct-runner.
…to enforce blank lines for imports and classdefs
Print correct line numbers for warnings.
lukecwik and others added 14 commits March 6, 2018 11:51
…eout

[BEAM-3775] Increase timeout in beam_PostCommit_Java_ValidatesRunner_…
….0-SNAPSHOT

Bump sdks/go/container/pom.xml to 2.5.0-SNAPSHOT
[BEAM-3791] Update version number in build_rules.gradle
…on all runners use to simplify the contract the user has to implement
… state method defaulting to the implementation all runners use to simplify the contract the user has to implement

[BEAM-3794] Make StateInternals short state method defaulting to the implementation all runners use to simplify the contract the user has to implement
This re-instantiates the Go SDK. The resulting situation is this:

-----A--A*----------------- [master]
    /    \       \    \
   /      A**     \    \
  /        \       \    \
-----*--*---*-------------- [go-sdk]

    A   accidental merge of Go SDK to master

    A*  revert of A; removes the Go SDK from any branch it is merged to

    A** revert of A* to put Go SDK back on the go-sdk branch

This makes all expected upstream merges and eventual
promotion of the Go SDK work as expected.
@lukecwik
Copy link
Member

lukecwik commented Mar 7, 2018

CC: @wcn3 @herohde @lostluck

@lukecwik lukecwik self-requested a review March 7, 2018 20:40
@lukecwik lukecwik self-assigned this Mar 7, 2018
Copy link
Member

@lukecwik lukecwik left a comment

Choose a reason for hiding this comment

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

I went through the same git steps to revert and re-revert to validate the change and looked at the merge tree.

@lukecwik
Copy link
Member

lukecwik commented Mar 7, 2018

Note that #4808 is the change to fix up the master branch.

@kennknowles
Copy link
Member Author

I'll wait for #4808 to go green and get merged to master before doing anything on this one, to be sure they keep compatible histories.

@wcn3
Copy link
Contributor

wcn3 commented Mar 7, 2018

I've validated the Go build still works, so LGTM from that perspective.

@kennknowles
Copy link
Member Author

Were the RAT failures pre-existing?

@lukecwik
Copy link
Member

lukecwik commented Mar 8, 2018

It turns out that the Go/Java precommits don't depend on running RAT so it is being skipped:

def javaPreCommitRoots = [

@kennknowles
Copy link
Member Author

Why is the precommit looped out and using those targets? Shouldn't it just be straight line hardcoded list of targets like :sdks:java:core:check and :globalRAT or some such?

@lukecwik
Copy link
Member

lukecwik commented Mar 8, 2018

Ken, please direct your comments on #4830

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.