Cherry picks for 0.14.0 branch #2333

fishy · 2021-02-18T03:35:34Z

Cherry pick the following PRs into 0.14.0 branch:

Client: py When no fields are present, we don't get the special constructor that uses __setattr__ to avoid these checks. So the default constructor sets message normally and triggers the anti-mutation tripwires.

Client: go This fixes the bug reported in apache#2315 (comment).

Jens-G · 2021-02-18T09:59:48Z

test/ThriftTest.thrift

@@ -411,3 +411,17 @@ struct StructB {
 struct OptionalSetDefaultTest {
  1: optional set<string> with_default = [ "test" ]
 }
+
+service ConflictingNamesTest {


The ThriftTest.thrift file serves a very specific purpose: It is the base for the cross language Test Suite.
Adding unrelated tests in that file is highly discouraged.

OK I'll create another PR to move this part out to a different thrift file, and if needed we can cherry pick that to 0.14.0 branch as well.

Also cherry picked into this branch (thus added into this PR).

Jens-G

See comment

Jens-G · 2021-02-18T10:02:59Z

Why do we need that as a hotfix in 0.14.0? Could you explain?

fishy · 2021-02-18T16:45:05Z

@Jens-G

Why do we need that as a hotfix in 0.14.0? Could you explain?

These 2 commits fix 2 regressions in 0.14.0:

If you have an exception without any fields, for example:

exception LookMaNoFields {}

You can create an instance of LookMaNoFields in python code with thrift 0.12.0 python library without a problem (I'm not 100% sure if that works with 0.13.0 python library), but with 0.14.0 python library you'll get TypeError: can't modify immutable instance exception. #2330 fixed that.

If you have an endpoint with an arg with name meta (like the test case I added to ThriftTest.thrift), thrift compiler 0.14.0 generated go code won't compile because of name conflicts. This used to work in 0.13.0 and it's fixed in THRIFT-4914: Fix name redeclaration bug in compiled go code #2332.

Jens-G · 2021-02-18T20:48:53Z

+1 then. Wish we would finds these kind of issues earlier next time.

fishy · 2021-02-18T21:20:58Z

These are all "corner cases" that only affect a small subset of use cases, so naturally they would only surface when more and more users start to adopt the new version. We didn't have enough people testing the 0.14.0 rc during the rc period.

For a process improvement, I would propose that the next time before the release, we actually do the full release for the rc/beta (e.g. have the v0.14.0-rc1 git tag, push the built artifacts to package managers, etc.). A lot of our users would only use the actual tagged "official" versions instead of just a branch or a certain commit, so that would help increase the real world coverage of the release candidate tests.

Jens-G · 2021-02-18T21:35:47Z

These are all "corner cases" that only affect a small subset of use cases,

Or bad testcase design. I tend to refuse to accept such explanations, because it opens the slope to banana software. Sure, shit happens (Forrest Gump) but as developers we should strive to not let it happen. We expect our stuff to work perfectly and are astonished when it does not.

The key problem with tests is, that developers are used to think constructive. But to write good test cases you have to be destructive. Pick the evil guy's hat and ask yourself: What would potentially break it? How can I bring that server down? Then put that it into a test case.

push the built artifacts to package managers

Problem is, we can't. The ASF has very clear rules: Everything that gets published has to be voted and accepted as a release. We can have internal pre-releases, but are not allowed to upload it to package managers. So if I don't overlook a thing (which is certainly possible) the only way would indeed be to have two official releases.

Publication

Projects SHALL publish official releases and SHALL NOT publish unreleased materials outside the development community.

During the process of developing software and preparing a release, various packages are made available to the development community for testing purposes. Projects MUST direct outsiders towards official releases rather than raw source repositories, nightly builds, snapshots, release candidates, or any other similar packages. The only people who are supposed to know about such developer resources are individuals actively participating in development or following the dev list and thus aware of the conditions placed on unreleased materials.

Jens-G · 2021-02-18T21:40:22Z

lib/go/test/tests/equals_test.go

+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ * Contains some contributions under the Thrift Software License.


Contains some contributions under the Thrift Software License.

Does it?

Jens-G

Overall LGTM, not tetsed. Except the addendum at the ASF header looks strange, Its not part of the normal ASF header and I wonder why you added it? Copypasta?

fishy · 2021-02-18T21:42:44Z

Overall LGTM, not tetsed. Except the addendum at the ASF header looks strange, Its not part of the normal ASF header and I wonder why you added it? Copypasta?

yea bad copy pasta. will fix.

fishy · 2021-02-18T21:59:54Z

@Jens-G what do you think about also cherry-pick #2335 into 0.14.0 branch? https://issues.apache.org/jira/browse/THRIFT-5353?focusedCommentId=17286691&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17286691 is an explanation of the issue.

Also I fixed the ASF headers.

Jens-G · 2021-02-18T22:12:26Z

+1

fishy · 2021-02-18T22:16:44Z

OK added #2335 into this PR.

Jens-G · 2021-02-19T07:53:40Z

lib/go/test/ConflictArgNamesTest.thrift

+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ * Contains some contributions under the Thrift Software License.


Contains some contributions under the Thrift Software License. ...

You forgot one :-)
Once fixed: Are you going to merge into the branch or are you expecting me to do that?

🤦

Removed. both here and in #2334

Client: go Also add missing copyright header for files added in apache#2307.

Client: go When a thrift file includes 2 or more other thrift files, and those included thrift files do not have explicit go namespaces defined, the current import dedup logic would wrongly use their empty namespace and skip the second one, while the real import namespace should be inferred from the filename.

fishy · 2021-02-22T17:13:29Z

@Jens-G did I address all your comments?

spladug and others added 2 commits February 17, 2021 19:30

THRIFT-5352: Fix construction of Py exceptions with no fields

ef98551

Client: py When no fields are present, we don't get the special constructor that uses __setattr__ to avoid these checks. So the default constructor sets message normally and triggers the anti-mutation tripwires.

THRIFT-4914: Fix name redeclaration bug in compiled go code

bc15861

Client: go This fixes the bug reported in apache#2315 (comment).

fishy requested a review from Jens-G February 18, 2021 03:35

Jens-G reviewed Feb 18, 2021

View reviewed changes

Jens-G self-requested a review February 18, 2021 10:01

Jens-G requested changes Feb 18, 2021

View reviewed changes

fishy requested a review from Jens-G February 18, 2021 18:40

fishy force-pushed the cherry-pick-0.14.0 branch from c3ff3ff to 7605806 Compare February 18, 2021 20:09

Jens-G reviewed Feb 18, 2021

View reviewed changes

Jens-G self-requested a review February 18, 2021 21:43

fishy force-pushed the cherry-pick-0.14.0 branch 2 times, most recently from 2bf0893 to c755f0b Compare February 18, 2021 21:45

Jens-G reviewed Feb 19, 2021

View reviewed changes

fishy added 2 commits February 19, 2021 08:38

Move ConflictingNamesTest to lib/go/test

84af215

Client: go Also add missing copyright header for files added in apache#2307.

fishy force-pushed the cherry-pick-0.14.0 branch from ccebb9f to e0786a4 Compare February 19, 2021 16:39

Jens-G approved these changes Feb 22, 2021

View reviewed changes

fishy merged commit e89b3e1 into apache:0.14.0 Feb 22, 2021

fishy deleted the cherry-pick-0.14.0 branch February 22, 2021 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cherry picks for 0.14.0 branch #2333

Cherry picks for 0.14.0 branch #2333

fishy commented Feb 18, 2021 •

edited

Loading

Jens-G Feb 18, 2021

fishy Feb 18, 2021

fishy Feb 18, 2021

fishy Feb 18, 2021

Jens-G left a comment

Jens-G commented Feb 18, 2021 •

edited

Loading

fishy commented Feb 18, 2021

Jens-G commented Feb 18, 2021

fishy commented Feb 18, 2021

Jens-G commented Feb 18, 2021 •

edited

Loading

Jens-G Feb 18, 2021 •

edited

Loading

Jens-G left a comment

fishy commented Feb 18, 2021

fishy commented Feb 18, 2021

Jens-G commented Feb 18, 2021

fishy commented Feb 18, 2021

Jens-G Feb 19, 2021

Jens-G Feb 19, 2021 •

edited

Loading

fishy Feb 19, 2021

fishy commented Feb 22, 2021

Cherry picks for 0.14.0 branch #2333

Cherry picks for 0.14.0 branch #2333

Conversation

fishy commented Feb 18, 2021 • edited Loading

Jens-G Feb 18, 2021

Choose a reason for hiding this comment

fishy Feb 18, 2021

Choose a reason for hiding this comment

fishy Feb 18, 2021

Choose a reason for hiding this comment

fishy Feb 18, 2021

Choose a reason for hiding this comment

Jens-G left a comment

Choose a reason for hiding this comment

Jens-G commented Feb 18, 2021 • edited Loading

fishy commented Feb 18, 2021

Jens-G commented Feb 18, 2021

fishy commented Feb 18, 2021

Jens-G commented Feb 18, 2021 • edited Loading

Jens-G Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Jens-G left a comment

Choose a reason for hiding this comment

fishy commented Feb 18, 2021

fishy commented Feb 18, 2021

Jens-G commented Feb 18, 2021

fishy commented Feb 18, 2021

Jens-G Feb 19, 2021

Choose a reason for hiding this comment

Jens-G Feb 19, 2021 • edited Loading

Choose a reason for hiding this comment

fishy Feb 19, 2021

Choose a reason for hiding this comment

fishy commented Feb 22, 2021

fishy commented Feb 18, 2021 •

edited

Loading

Jens-G commented Feb 18, 2021 •

edited

Loading

Jens-G commented Feb 18, 2021 •

edited

Loading

Jens-G Feb 18, 2021 •

edited

Loading

Jens-G Feb 19, 2021 •

edited

Loading