[GitHub] thrift issue #1474: Recognize fbthrift TApplicationException codes
Github user jfarrell commented on the issue: https://github.com/apache/thrift/pull/1474 Hi @yfeldblum, thanks for taking a look into this for us, appreciate your help. A new PR would be great that way it gives the ASF the needed audit trail of the origination of the pull request code contribution coming from Facebook to the ASF. ---
[GitHub] thrift issue #1474: Recognize fbthrift TApplicationException codes
Github user yfeldblum commented on the issue: https://github.com/apache/thrift/pull/1474 @jfarrell Does Apache actually need a new PR originating from FB? Or is it sufficient for FB to okay this PR? ---
[GitHub] thrift issue #1474: Recognize fbthrift TApplicationException codes
Github user jfarrell commented on the issue: https://github.com/apache/thrift/pull/1474 We can not directly accept this PR as it contains code that originated in fbthrift. We need someone from Facebook to contribute the code or files back to Apache Thrift for use by the ASF. @djwatson if you have some free time would you mind contributing back the recent updates to fbthrift in the cpp lib around the Application and Transport Exception headers to help resolve this issue? Appreciate your time and help as always. - https://github.com/facebook/fbthrift/blob/master/thrift/lib/cpp/TApplicationException.h - https://github.com/facebook/fbthrift/blob/master/thrift/lib/cpp/transport/TTransportException.h ---
[GitHub] thrift issue #1474: Recognize fbthrift TApplicationException codes
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1474 @jfarrell can you please give a final say on this? ---
[GitHub] thrift issue #1474: Recognize fbthrift TApplicationException codes
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1474 @jfarrell can you please give a final say on this? ---
[GitHub] thrift issue #1474: Recognize fbthrift TApplicationException codes
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1474 Based on my read of the file from which the original source information came, the original fbthrift project files are Apache Source licensed. So I'm not certain we need to do anything in particular, but @jfarrell said that we do. As such, I am going to wait until he gives the okay. ---
[GitHub] thrift issue #1474: Recognize fbthrift TApplicationException codes
Github user jparise commented on the issue: https://github.com/apache/thrift/pull/1474 Coming back to the original proposed change, is this likely or unlikely to be merged? I don't think I fully understand @jfarrell's "this change must originate from Facebook" requirement. Both projects are Apache 2.0-licensed, which may or may not be relevant if the concern is a related to copyright. I wrote all of the code in this diff myself, from scratch, using only the fbthrift enumeration names and integer values as a reference. I haven't run a `diff` between the two trees to see how similar the individual lines of code may or may not be, but I'm not sure how relevant that would be given that enumerations can only be written so many ways. Of course, I am not a lawyer, etc., so if this is a blocker for inclusion in Apache Thrift, please say so. ---
[GitHub] thrift issue #1474: Recognize fbthrift TApplicationException codes
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1474 @jfarrell from what I can see these error codes are inside contributed code in the fbthrift project, at least from the C++ side, in a file that contains the Apache license: https://github.com/facebook/fbthrift/blob/master/thrift/lib/cpp/transport/TTransportException.h Making error codes generated as part of the build process would actually be pretty cool; there would be some sort of top level directory to store common thrift (perhaps lib/common/) and inside a Transport.thrift file (or something like that) with an enumeration and a LOT of namespace directives... and the build would generate the language-specific output it needs to build the library. Sounds pretty cool. ---
[GitHub] thrift issue #1474: Recognize fbthrift TApplicationException codes
Github user jfarrell commented on the issue: https://github.com/apache/thrift/pull/1474 @jeking3 `Do we really need to ask for permission to add three additional error codes from fbthrift?` - yes, if there is something in Facebook fbthrift that someone wants to add to Apache Thrift it has to come as a contribution from Facebook. @Jens-G I cant recall a ticket for it either, but agree with having the common portion built into the generator to reduce updating many touch points in the code ---
[GitHub] thrift issue #1474: Recognize fbthrift TApplicationException codes
Github user Jens-G commented on the issue: https://github.com/apache/thrift/pull/1474 Not sure. We had that discussion in the past IIRC and the outcome was that we leave it as it is. It's somewhere buried in the archives, maybe we heven had (or still have) a ticket for it. Nevertheless, the idea has some clear advantages, I agreee with that. One question that immediately came to my mind was: why don't we include the version number as well? That thing is a constant hassle with any release. @jfarrell ? ---
[GitHub] thrift issue #1474: Recognize fbthrift TApplicationException codes
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1474 @Jens-G what do you think about the notion of moving the definition of error message enumerations into a single common .thrift file that gets compiled and then comsumed by the libraries? It would certainly go a long way to ensuring that these error codes are handled in a standardized way across all languages. I think that the generated code would have to be packaged along with the built library in some cases, but that shouldn't be a problem. ---
[GitHub] thrift issue #1474: Recognize fbthrift TApplicationException codes
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1474 In general I frown upon adding test-only values in production code (like "injected failure"). That said I see no reason for us not to incorporate these into the core. Do we really need to ask for permission to add three additional error codes from fbthrift? This may not have properly gotten every language (Common Lisp comes to mind, as it's an open PR). It's too bad we don't define the list of error codes in Thrift and then compile them for every language to use... it would ensure uniformity across languages... ---
[GitHub] thrift issue #1474: Recognize fbthrift TApplicationException codes
Github user jparise commented on the issue: https://github.com/apache/thrift/pull/1474 I'm not affiliated with either project, but I work in an environment that uses multiple Thrift implementations, so interoperability is important to us. We're specifically interested in using some of these new codes (e.g. `LOADSHEDDING`) between services, and I think there's value in achieving error code parity. Likewise, if Apache Thrift had been the first to introduce these new error codes, I'd be making the same case for their inclusion in fbthrift and other implementations. I don't know if it's important to establish a plan for "approving" new error codes going forward. When I wrote "guarantee no future conflicts" in the summary, I meant that these particular values (11, 12, 13) have already been effectively allocated by fbthrift, and adding them to Apache Thrift eliminates the chance that those indices are ever assigned to different, conflicting codes in this project. ---
[GitHub] thrift issue #1474: Recognize fbthrift TApplicationException codes
Github user Jens-G commented on the issue: https://github.com/apache/thrift/pull/1474 I see the point but I'm not sure if Inunderstzand it right. Who doies what before we add new codes? We ask FB? Or they ask us? And what ist your plan to convince FB that they have to ask us? ---
