[GitHub] thrift issue #1526: Merge pull request #1 from apache/master
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1526 Did you mean to make this PR? If so, why? ---
[GitHub] thrift issue #1516: THRIFT-4527 Upgrade byteorder in rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1516 OK. Looks like the build failures are unrelated. I'm going to apply locally and check cross-tests just to make sure, but will get this applied today. Thank you @QuestofIranon! Always happy to see people improving Thrift :) ---
[GitHub] thrift issue #1516: THRIFT-4527 Upgrade byteorder in rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1516 LGTM. @QuestofIranon just curious - why bump the version? ---
[GitHub] thrift issue #1269: THRIFT-4187 Allow dart framed transport to read incomple...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1269 @jeking3 Yup - just did that. Is there a way to do it automatically, or is it a manual process (for now)? ---
[GitHub] thrift issue #1269: THRIFT-4187 Allow dart framed transport to read incomple...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1269 No problem! I'm sorry I was lax and left it so long :/ Looks like the tests passed, so it's good to merge. Not 100% of the process, so I'll follow up via email. ---
[GitHub] thrift issue #1269: THRIFT-4187 Allow dart framed transport to read incomple...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1269 @jeking3 Any chance this could be merged? TY! ---
[GitHub] thrift issue #1269: THRIFT-4187 Allow dart framed transport to read incomple...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1269 @jeking3 I've added unit tests around this behavior and enabled more cross tests! Do you want me to roll that into this PR, or should I create another bug and PR? ---
[GitHub] thrift issue #1269: THRIFT-4187 Allow dart framed transport to read incomple...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1269 @jeking3 Fixed up the failing tests. Confirming that precross works. ---
[GitHub] thrift pull request #1508: THRIFT-4419: Fix bug where framed messages > 4K c...
GitHub user allengeorge opened a pull request: https://github.com/apache/thrift/pull/1508 THRIFT-4419: Fix bug where framed messages > 4K could not be read Client: rs You can merge this pull request into a Git repository by running: $ git pull https://github.com/allengeorge/thrift thrift-4419 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1508.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1508 commit f358ffbd997d33ad60bf4602c65d46ea6792386a Author: Allen George <allen.george@...> Date: 2017-12-13T12:34:49Z THRIFT-4419: Fix bug where framed messages > 4K could not be read Client: rs ---
[GitHub] thrift issue #1269: THRIFT-4187 Allow dart framed transport to read incomple...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1269 Yup. Will do. I'm a gonna set myself a target to get it done by end of next week. I've been lax on this :/ ---
[GitHub] thrift issue #1475: THRIFT-4464: Fix typo in TNonblockingServer.py
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1475 On the face of it that looks correct. There isn't even a `message` in scope, so when it hits that line the code would just break. I'll look a little deeper. ---
[GitHub] thrift issue #1458: THRIFT-4390: Fix bug where binary/buffered messages > 4K...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1458 @jeking3 Looks like everything passed, so this patch is good to go. TY! ---
[GitHub] thrift issue #1458: THRIFT-4390: Fix bug where binary/buffered messages > 4K...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1458 @jeking3 Thank you! So...updated the PR to re-add the three failing tests. Created [THRIFT-4451](https://issues.apache.org/jira/browse/THRIFT-4451) to track this. I'm going to fix up the framed transport first, circle back to 4451, and then handle dart/c++ issues. ---
[GitHub] thrift issue #1458: THRIFT-4390: Fix bug where binary/buffered messages > 4K...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1458 @jeking3 This seems like a separate problem unfortunately. I'd removed three tests from the "known failures list" and it appears there's a specific problem related to multiplexed processors and c_glib/perl. That was an oversight :/ I'd suggest the following course of action: 1. I re-add those three test failures to the "known failures" list 2. I fix up the framed test failures with > 4k long messages; I have a patch for this already 3. I add a JIRA for the multiplexed failures and fix that in a separate PR Let me know if this is acceptable to you, and I'll go about doing that. ---
[GitHub] thrift issue #1458: THRIFT-4390: Fix bug where binary/buffered messages > 4K...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1458 @jeking3 This seems like a separate problem unfortunately. I'd removed three tests from the "known failures list" and it appears there's a specific problem related to multiplexed processors and c_glib/perl. That was an oversight :/ I'd suggest the following course of action: 1. I re-add those three test failures to the "known failures" list 2. I fix up the framed test failures with > 4k long messages; I have a patch for this already 3. I add a JIRA for the multiplexed failures and fix that in a separate PR Let me know if this is acceptable to you, and I'll go about doing that. ---
[GitHub] thrift issue #1458: THRIFT-4390: Fix bug where binary/buffered messages > 4K...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1458 One of two fixes for problems noticed by others during cross-tests. I've a separate PR for framed transports. ---
[GitHub] thrift issue #1269: THRIFT-4187 Allow dart framed transport to read incomple...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1269 Hi James - Iâm really sorry for the delay; slammed at work. Iâll work on it this weekend. Again, super sorry about this From: James E. King, III <notificati...@github.com> Sent: Wednesday, October 25, 2017 10:44:19 AM To: apache/thrift Cc: Allen George; Mention Subject: Re: [apache/thrift] THRIFT-4187 Allow dart framed transport to read incomplete frame (#1269) @allengeorge<https://github.com/allengeorge> if you add framed transport in dart to the cross test, that would test a lot... but it would not test error conditions such as this one. Just a reminder, this needs to move forward. â You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://github.com/apache/thrift/pull/1269#issuecomment-339353877>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAEO9HLOiegU2gF0E4zeUS7XjKsp5ZJlks5sv0lDgaJpZM4NaaLl>. ---
[GitHub] thrift issue #1360: THRIFT-4330: Allow unused extern crates
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1360 @sadikovi Sorry for the delay - was away this weekend. Ah, ok. That makes total sense. You probably don't have any floats in your IDL, or anything that would use the `TryFrom` conversion. LGTM. @Jens-G LGTM; could you merge this please? TY! ---
[GitHub] thrift issue #1360: THRIFT-4330: Allow unused extern crates
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1360 @sadikovi The change seems fine, but I don't quite understand when this problem happens. Could you explain when it gets triggered? In what sort of code setup? Thanks! ---
[GitHub] thrift issue #1269: THRIFT-4187 Allow dart framed transport to read incomple...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1269 @markerickson-wf My bad - I should definitely have run the unit tests. I'll check them and fix them up. Also, I'll look into how to create a reasonable set of unit tests for the framed transport. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1269: THRIFT-4187 Allow dart framed transport to read i...
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1269#discussion_r116517412
--- Diff: lib/dart/lib/src/transport/t_framed_transport.dart ---
@@ -51,33 +58,112 @@ class TFramedTransport extends TBufferedTransport {
if (got > 0) return got;
}
-_readFrame();
+// IMPORTANT: by the time you've got here,
+// an entire frame is available for reading
return super.read(buffer, offset, length);
}
void _readFrame() {
-_transport.readAll(headerBytes, 0, headerByteCount);
-int size = headerBytes.buffer.asByteData().getUint32(0);
+if (_body == null) {
+ bool gotFullHeader = _readFrameHeader();
+ if (!gotFullHeader) {
+return;
+ }
+}
+
+_readFrameBody();
+ }
+
+ bool _readFrameHeader() {
+var remainingHeaderBytes = headerByteCount - _receivedHeaderBytes;
-if (size < 0) {
+int got = _transport.read(_headerBytes, _receivedHeaderBytes,
remainingHeaderBytes);
+if (got < 0) {
throw new TTransportError(
- TTransportErrorType.UNKNOWN, "Read a negative frame size:
$size");
+ TTransportErrorType.UNKNOWN, "Socket closed during frame header
read");
}
-Uint8List buffer = new Uint8List(size);
-_transport.readAll(buffer, 0, size);
-_setReadBuffer(buffer);
+_receivedHeaderBytes += got;
+
+if (_receivedHeaderBytes == headerByteCount) {
+ int size = _headerBytes.buffer.asByteData().getUint32(0);
+
+ _receivedHeaderBytes = 0;
+
+ if (size < 0) {
+throw new TTransportError(
+TTransportErrorType.UNKNOWN, "Read a negative frame size:
$size");
+ }
+
+ _bodySize = size;
+ _body = new Uint8List(_bodySize);
+ _receivedBodyBytes = 0;
+
+ return true;
+} else {
+ _registerForReadableBytes();
--- End diff --
It shouldn't cause a cycle, but I may have missed something? It's the first
time I've done anything with Dart.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1269: THRIFT-4187 Allow dart framed transport to read i...
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1269#discussion_r116517213
--- Diff: lib/dart/lib/src/transport/t_framed_transport.dart ---
@@ -51,33 +58,112 @@ class TFramedTransport extends TBufferedTransport {
if (got > 0) return got;
}
-_readFrame();
+// IMPORTANT: by the time you've got here,
+// an entire frame is available for reading
return super.read(buffer, offset, length);
}
void _readFrame() {
-_transport.readAll(headerBytes, 0, headerByteCount);
-int size = headerBytes.buffer.asByteData().getUint32(0);
+if (_body == null) {
+ bool gotFullHeader = _readFrameHeader();
+ if (!gotFullHeader) {
+return;
+ }
+}
+
+_readFrameBody();
+ }
+
+ bool _readFrameHeader() {
+var remainingHeaderBytes = headerByteCount - _receivedHeaderBytes;
-if (size < 0) {
+int got = _transport.read(_headerBytes, _receivedHeaderBytes,
remainingHeaderBytes);
+if (got < 0) {
throw new TTransportError(
- TTransportErrorType.UNKNOWN, "Read a negative frame size:
$size");
+ TTransportErrorType.UNKNOWN, "Socket closed during frame header
read");
}
-Uint8List buffer = new Uint8List(size);
-_transport.readAll(buffer, 0, size);
-_setReadBuffer(buffer);
+_receivedHeaderBytes += got;
+
+if (_receivedHeaderBytes == headerByteCount) {
+ int size = _headerBytes.buffer.asByteData().getUint32(0);
+
+ _receivedHeaderBytes = 0;
+
+ if (size < 0) {
+throw new TTransportError(
+TTransportErrorType.UNKNOWN, "Read a negative frame size:
$size");
+ }
+
+ _bodySize = size;
+ _body = new Uint8List(_bodySize);
+ _receivedBodyBytes = 0;
+
+ return true;
+} else {
+ _registerForReadableBytes();
+ return false;
+}
+ }
+
+ void _readFrameBody() {
+var remainingBodyBytes = _bodySize - _receivedBodyBytes;
+
+int got = _transport.read(_body, _receivedBodyBytes,
remainingBodyBytes);
+if (got < 0) {
+ throw new TTransportError(
+ TTransportErrorType.UNKNOWN, "Socket closed during frame body
read");
+}
+
+_receivedBodyBytes += got;
+
+if (_receivedBodyBytes == _bodySize) {
+ var body = _body;
+
+ _bodySize = 0;
+ _body = null;
+ _receivedBodyBytes = 0;
+
+ _setReadBuffer(body);
+
+ var completer = _frameCompleter;
+ _frameCompleter = null;
+ completer.complete(new Object());
+} else {
+ _registerForReadableBytes();
+}
}
Future flush() {
-Uint8List buffer = consumeWriteBuffer();
-int length = buffer.length;
+if (_frameCompleter == null) {
+ Uint8List buffer = consumeWriteBuffer();
+ int length = buffer.length;
+
+ _headerBytes.buffer.asByteData().setUint32(0, length);
+ _transport.write(_headerBytes, 0, headerByteCount);
+ _transport.write(buffer, 0, length);
+
+ _frameCompleter = new Completer(); // FIXME: .sync?!
--- End diff --
Ah. Actually I need advice from you as to whether this has to be a `sync`
`Completer` or not :) It's not quite clear to me when one would choose one, and
whether that's necessary at this point.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1269: THRIFT-4187 Allow dart framed transport to read i...
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1269#discussion_r116516987
--- Diff: lib/dart/lib/src/transport/t_framed_transport.dart ---
@@ -51,33 +58,112 @@ class TFramedTransport extends TBufferedTransport {
if (got > 0) return got;
}
-_readFrame();
+// IMPORTANT: by the time you've got here,
+// an entire frame is available for reading
return super.read(buffer, offset, length);
}
void _readFrame() {
-_transport.readAll(headerBytes, 0, headerByteCount);
-int size = headerBytes.buffer.asByteData().getUint32(0);
+if (_body == null) {
+ bool gotFullHeader = _readFrameHeader();
+ if (!gotFullHeader) {
+return;
+ }
+}
+
+_readFrameBody();
+ }
+
+ bool _readFrameHeader() {
+var remainingHeaderBytes = headerByteCount - _receivedHeaderBytes;
-if (size < 0) {
+int got = _transport.read(_headerBytes, _receivedHeaderBytes,
remainingHeaderBytes);
+if (got < 0) {
throw new TTransportError(
- TTransportErrorType.UNKNOWN, "Read a negative frame size:
$size");
+ TTransportErrorType.UNKNOWN, "Socket closed during frame header
read");
}
-Uint8List buffer = new Uint8List(size);
-_transport.readAll(buffer, 0, size);
-_setReadBuffer(buffer);
+_receivedHeaderBytes += got;
+
+if (_receivedHeaderBytes == headerByteCount) {
+ int size = _headerBytes.buffer.asByteData().getUint32(0);
+
+ _receivedHeaderBytes = 0;
+
+ if (size < 0) {
+throw new TTransportError(
+TTransportErrorType.UNKNOWN, "Read a negative frame size:
$size");
+ }
+
+ _bodySize = size;
+ _body = new Uint8List(_bodySize);
+ _receivedBodyBytes = 0;
+
+ return true;
+} else {
+ _registerForReadableBytes();
+ return false;
+}
+ }
+
+ void _readFrameBody() {
+var remainingBodyBytes = _bodySize - _receivedBodyBytes;
+
+int got = _transport.read(_body, _receivedBodyBytes,
remainingBodyBytes);
+if (got < 0) {
+ throw new TTransportError(
+ TTransportErrorType.UNKNOWN, "Socket closed during frame body
read");
+}
+
+_receivedBodyBytes += got;
+
+if (_receivedBodyBytes == _bodySize) {
+ var body = _body;
+
+ _bodySize = 0;
+ _body = null;
+ _receivedBodyBytes = 0;
+
+ _setReadBuffer(body);
+
+ var completer = _frameCompleter;
+ _frameCompleter = null;
+ completer.complete(new Object());
--- End diff --
I was a little worried about returning the frame bytes, but I guess it
doesn't matter. Also, I assume that each call to `iterator` returns a
completely separate iterator with a view to the underlying buffer.
I'll change it. BTW, it's a little weird that `flush()` returns bytes...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1269: THRIFT-4187 Allow dart framed transport to read i...
GitHub user allengeorge opened a pull request: https://github.com/apache/thrift/pull/1269 THRIFT-4187 Allow dart framed transport to read incomplete frame You can merge this pull request into a Git repository by running: $ git pull https://github.com/allengeorge/thrift allen/thrift-4187-final Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1269.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1269 commit ebcc53c36cdd34d7d7c33a849ebda1cff3aa7241 Author: Allen George <allen.geo...@gmail.com> Date: 2017-05-13T17:41:20Z THRIFT-4187 Allow dart framed transport to read incomplete frame --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1267: THRIFT-4196 Support recursive types in Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1267 @Jens-G Any chance this could go in? Thank you? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1267: THRIFT-4196 Support recursive types in Rust
GitHub user allengeorge opened a pull request: https://github.com/apache/thrift/pull/1267 THRIFT-4196 Support recursive types in Rust Client: rs * Modify code generator to support recursive types * Modify rust test (not cross-test) client to test recursive calls You can merge this pull request into a Git repository by running: $ git pull https://github.com/allengeorge/thrift allen/thrift-4196-final Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1267.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1267 commit a3e38261108430585f0e5d72f59b093e1ceffbf6 Author: Allen George <allen.geo...@gmail.com> Date: 2017-05-11T11:56:15Z THRIFT-4196 Support recursive types in Rust Client: rs * Modify code generator to support recursive types * Modify rust test (not cross-test) client to test recursive calls --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1260 @Jens-G Thank you! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1260 @jeking3 Thanks for pointing me in the right direction. Everything now passes! I realize you're not committing, but would it be possible for one of the other maintainers - @Jens-G perhaps? - to get this change in? To summarize: * Added rust cross-test and unit-test support to travis * Added rust multiplexed client/server cross-tests; fixed some bugs in my multiplexing impl. * Split up test matrix to avoid timeouts * Discovered bug in Dart framed transport implementation; filed [THRIFT-4187](https://issues.apache.org/jira/browse/THRIFT-4187) So, with the exception of the two dart tests, every other rust cross-test and all unit tests pass. I've also fully linted and formatted the rust code. Thank you all! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1260 @jeking3 Oh. Totally didn't realize that - thanks for clarifying! I've updated my `TMultiplexedProcessor` implementation to have a backwards-compatibility mode, and am re-running the build right now. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1252: [Ruby] Logging server-side exception message on Applicat...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1252 @jeking3 If you're willing to take this...I have some ruby experience, and this looks good to me. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1260 @Jens-G @jeking3 Can I get any guidance on how the clients/server get their arguments? I'm confident it's not a bug in the rust server. I can invoke the c_glib client and rust server manually on the command line and they both communicate properly. Somehow the c_glib client gets `binary` while the rust server gets `multi` (and the corresponding arguments for compact). I looked into `crossrunner/collect.py` but the code is a little tough to decipher, and it's not clear how it's determining an intersection. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1260 OK. I really don't understand `tests.json`. I'm confident that my code is doing the right thing - it's just that the server is invoked in multiplexed mode and the client is invoked in non-multiplexed mode. Super bizarre. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1260 Ah. Gotcha. Sorry - I should have realized that. I'll pay closer attention next time. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1260 I had to split up binary/compact protocols because the builds were timing out with them together. And, AFAICT, the unit tests aren't running for builds 1 and 2. Also (I didn't change this) but I'm unsure of the utility of some of the builds in the matrix: - TEST_NAME="C C++ C# D Erlang Haxe Go (automake)" - TEST_NAME="C C++ Plugin Haskell Perl - GCC (automake)" - TEST_NAME="Java Lua PHP Ruby Dart Node.js Python (automake)" Looks like these are actually running the unit tests? If so, then why is C/C++ repeated? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1260 I've rebalanced the matrix. Trying again. @jeking3 Having a weird problem with the c_glib client. It's using the "binary" or "compact" protocol only when invoked in multi/multic mode. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1260 @jeking3 One hopes that...congratulations are in order? :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1260 Please pull the latest commit. I updated to fix a bug in my test server. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1260: THRIFT-4186 Add travis build for Rust
Github user allengeorge commented on a diff in the pull request: https://github.com/apache/thrift/pull/1260#discussion_r114546199 --- Diff: test/known_failures_Linux.json --- @@ -224,5 +224,7 @@ "hs-py3_json_framed-ip", "java-d_compact_buffered-ip", "java-d_compact_buffered-ip-ssl", - "java-d_compact_framed-ip" + "java-d_compact_framed-ip", + "rs-dart_binary_framed-ip", --- End diff -- Oh. If you're talking about a JIRA, I've already filed one: [THRIFT-4187](https://issues.apache.org/jira/browse/THRIFT-4187) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1260 @jeking3 Happy to make the change to the travis jobs! Let me modify this PR with the defect change and then work on rebalancing the travis jobs. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1260: THRIFT-4186 Add travis build for Rust
Github user allengeorge commented on a diff in the pull request: https://github.com/apache/thrift/pull/1260#discussion_r114530042 --- Diff: test/known_failures_Linux.json --- @@ -224,5 +224,7 @@ "hs-py3_json_framed-ip", "java-d_compact_buffered-ip", "java-d_compact_buffered-ip-ssl", - "java-d_compact_framed-ip" + "java-d_compact_framed-ip", + "rs-dart_binary_framed-ip", --- End diff -- OK. Will do. Didn't realize there was a defect list. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1260: THRIFT-4186 Add travis build for Rust
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1260#discussion_r114529981
--- Diff: test/rs/src/bin/test_server.rs ---
@@ -104,35 +104,43 @@ fn run() -> thrift::Result<()> {
}
};
-let processor = ThriftTestSyncProcessor::new(ThriftTestSyncHandlerImpl
{});
+let test_processor =
ThriftTestSyncProcessor::new(ThriftTestSyncHandlerImpl {});
-let mut server = match &*server_type {
-"simple" => {
-TServer::new(
+match &*server_type {
+"simple" | "thread-pool" => {
+let mut server = TServer::new(
i_transport_factory,
i_protocol_factory,
o_transport_factory,
o_protocol_factory,
-processor,
+test_processor,
1,
-)
+);
+
+server.listen(_address)
}
-"thread-pool" => {
-TServer::new(
+t if t.to_owned().starts_with("multi") => {
+let second_service_processor =
SecondServiceSyncProcessor::new(SecondServiceSyncHandlerImpl {},);
+
+let mut multiplexed_processor = TMultiplexedProcessor::new();
+multiplexed_processor.register("ThriftTest",
Box::new(test_processor));
+multiplexed_processor.register("SecondService",
Box::new(second_service_processor));
--- End diff --
Glad to add it :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1260 No problem - I can change it to do that. Should that be in a different commit/pull-request, or merged in this one? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1260 Also, I'm fixing the CentOS issues. I mistakenly assumed that `sh` would be the same in both ubuntu and Centos. Guess I was wrong... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1260 @jeking3 I've updated the travis build with rust support, but I think I'm running into timeouts because now the builds take too long :/ Any ideas what I should do? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1260: THRIFT-4186 Add travis build for Rust
GitHub user allengeorge opened a pull request: https://github.com/apache/thrift/pull/1260 THRIFT-4186 Add travis build for Rust Client: rs * Install Rust in ubuntu/centos docker images * Fix type bounds on TMultiplexedProcessor * Add multiplexing support to cross-test server and client * Run multiplexed tests in rs cross-test * Temporarily add dart->rs framed as "known failures" You can merge this pull request into a Git repository by running: $ git pull https://github.com/allengeorge/thrift thrift-4186 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1260.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1260 commit b1f701a112768b29ede23a1e3d109933b4750965 Author: Allen George <allen.geo...@gmail.com> Date: 2017-04-28T14:22:03Z THRIFT-4186 Add travis build for Rust Client: rs * Install Rust in ubuntu/centos docker images * Fix bounds on TMultiplexedProcessor * Add multiplexing support to cross-test server and client * Run multiplexed tests in rs cross-test * Temporarily add dart->rs framed "known failure" --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1255: THRIFT-4176: Implement threaded server for Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1255 @jeking3 That's a very fair point - I didn't realize the cross-tests weren't running. I try to be disciplined in my testing and ensure nothing breaks, but I'm only human; automating things is the right way to go. I'll look at the c_glib example and update the docker images to run the cross tests. And, I agree that it would be nice to have a second person knowledgeable in the language. I'm not sure how best to make that happen. Any ideas? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1255: THRIFT-4176: Implement threaded server for Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1255 Updated the PR description with an example. This PR now contains the final state of all types, etc and should make the runtime code a lot cleaner and easier to use! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1255: THRIFT-4176: Implement threaded server for Rust
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1255 @jeking3 @Jens-G Any chance this could be merged in? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1255: THRIFT-4176: Implement threaded server for Rust
GitHub user allengeorge opened a pull request: https://github.com/apache/thrift/pull/1255 THRIFT-4176: Implement threaded server for Rust Client: rs * Separate TTransport into two constructs: TIoChannel and TTransport * Make TIoChannel and TTransport Send-able types * Remove refcounting where possible * Replace TSimpleServer with a thread-pool based TServer The removal of refcounting was driven by the need to make constructs threadsafe. I'd always known the ref-counting was hacky; this approach pushes the counting/locking into a separate `TIoChannel` concept that can be cloned to simplify the ownership story for all the concepts above. Also, as requested, I didn't *add* a new server type: I replaced the existing one with this threadpool-based version. You can merge this pull request into a Git repository by running: $ git pull https://github.com/allengeorge/thrift thrift-4176 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1255.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1255 commit 0a0592f333ea4c9be64dd83fa7ce6e78c7307567 Author: Allen George <allen.geo...@gmail.com> Date: 2017-01-30T12:15:00Z THRIFT-4176: Implement threaded server for Rust Client: rs * Separate TTransport into two constructs: TIoChannel and TTransport * Make TIoChannel and TTransport Send-able types * Remove refcounting where possible * Replace TSimpleServer with a thread-pool based TServer --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1187: Review for pull up of RUST extended namespace support, a...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1187 Yes please - that would be the best course of action. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1246: THRIFT-4099: Derive Hash trait for Rust structs
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1246 @jeking3 I did, though unfortunately the communication was all on [the original Rust Thrift](https://issues.apache.org/jira/browse/THRIFT-2945). I suggested that Tony - the submitter - split up his changes into separate PRs to simplify review and testing. I also created [individual JIRAs](https://issues.apache.org/jira/browse/THRIFT-2945?focusedCommentId=15878212=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15878212) for each of the pieces we agreed to. Unfortunately it looks like he went AWOL after that :/ I'm now attacking those PRs one by one. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1246: THRIFT-4099: Derive Hash trait for Rust structs
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1246 Hmm. Don't know who should review this, considering that I'm the one who implemented Rust support. But, FWIW, ran the standard cross-tests, checked clippy output, etc. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1226: THRIFT-4147: Rust: protocol should accept transports wit...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1226 Note that I've tested it against master, and all the rust tests and cross-tests pass. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1226: THRIFT-4147: Rust: protocol should accept transports wit...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1226 @Jens-G This looks good to me. Can it be merged? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1226: THRIFT-4147: Rust: protocol should accept transports wit...
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1226 Will take a look today. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 Yay! Build passed :) Any thoughts on whether this is OK to merge? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 Fixed makefile errors with `make distdir` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 More updates: - Minor cleanups to Rust code generators - Confirmed that all Rust code does not trigger any clippy lint warnings --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 @anatol Code has been updated with the following changes: - All usage of `try!` converted to `?` - Fix all but two clippy lint warnings (I thought the existing code was clearer) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 @anatol Thanks for the clippy output! I'll look through all of the items and fix them. I'd no idea that `new()` without `Default` was considered a code smell. Interesting. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r97211560
--- Diff: lib/rs/src/errors.rs ---
@@ -0,0 +1,678 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::convert::{From, Into};
+use std::error::Error as StdError;
+use std::fmt::{Debug, Display, Formatter};
+use std::{error, fmt, io, string};
+use try_from::TryFrom;
+
+use ::protocol::{TFieldIdentifier, TInputProtocol, TOutputProtocol,
TStructIdentifier, TType};
+
+// FIXME: should all my error structs impl error::Error as well?
+// FIXME: should all fields in TransportError, ProtocolError and
ApplicationError be optional?
+
+/// Thrift error type.
+///
+/// The error type defined here is used throughout this crate as well as in
+/// all Thrift-compiler-generated Rust code. It falls into one of four
+/// standard (i.e. defined by convention in Thrift implementations)
categories.
+/// These are:
+///
+/// 1. **Transport errors**: errors encountered while writing byes to the
I/O
+///channel. These include "connection closed", "bind errors", etc.
+/// 2. **Protocol errors**: errors encountered when processing a Thrift
message
+///within the thrift library. These include "exceeding size limits",
+///"unsupported protocol versions", etc.
+/// 3. **Application errors**: errors encountered when
Thrift-compiler-generated
+///'application' code encounters conditions that violate the spec.
These
+///include "out-of-order messages", "missing required-field values",
etc.
+///This category also functions as a catch-all: any error returned by
+///handler functions is automatically encoded as an `ApplicationError`
and
+///returned to the caller.
+/// 4. **User errors**: exception structs defined within the Thrift IDL.
+///
+/// With the exception of `Error::User`, each error variant encapsulates a
+/// corresponding struct which encodes two required fields:
+///
+/// 1. kind: an enum indicating what kind of error was encountered
+/// 2. message: string with human-readable error information
+///
+/// These two fields are encoded and sent over the wire to the remote.
`kind`
+/// is defined by convention while `message` is freeform. If none of the
+/// enumerated error kinds seem suitable use the catch-all `Unknown`.
+///
+/// # Examples
+///
+/// Creating a `TransportError` explicitly.
+///
+/// Conversions are defined from `thrift::TransportError`,
`thrift::ProtocolError`
+/// and `thrift::ApplicationError` to the corresponding `thrift::Error`
variant
+/// (see `err2` below).
+///
+/// ```
+/// use thrift;
+/// use thrift::{TransportError, TransportErrorKind};
+///
+/// let err0: thrift::Result<()> = Err(
+/// thrift::Error::Transport(
+/// TransportError {
+/// kind: TransportErrorKind::TimedOut,
+/// message: format!("connection to server timed out")
+/// }
+/// )
+/// );
+///
+/// let err1: thrift::Result<()> = Err(
+/// thrift::Error::Transport(
+/// TransportError::new(
+/// TransportErrorKind::TimedOut,
+/// format!("connection to server timed out")
+/// )
+/// )
+/// );
+///
+/// let err2: thrift::Result<()> = Err(
+/// thrift::Error::from(
+/// TransportError::new(
+/// TransportErrorKind::TimedOut,
+/// format!("connection to server timed out")
+/// )
+/// )
+/// );
+///
+/// ```
+///
+/// Creating an arbitrary error from a string
+///
+/// ```
+/// use thrift;
+/// use thrift::{ApplicationError, ApplicationErrorKind};
+///
+/// let err0: thrift::Result<()> = Err(
+/// thrift::Error::from("This is an error")
+
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r97211074
--- Diff: lib/rs/src/errors.rs ---
@@ -0,0 +1,678 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::convert::{From, Into};
+use std::error::Error as StdError;
+use std::fmt::{Debug, Display, Formatter};
+use std::{error, fmt, io, string};
+use try_from::TryFrom;
+
+use ::protocol::{TFieldIdentifier, TInputProtocol, TOutputProtocol,
TStructIdentifier, TType};
+
+// FIXME: should all my error structs impl error::Error as well?
+// FIXME: should all fields in TransportError, ProtocolError and
ApplicationError be optional?
+
+/// Thrift error type.
+///
+/// The error type defined here is used throughout this crate as well as in
+/// all Thrift-compiler-generated Rust code. It falls into one of four
+/// standard (i.e. defined by convention in Thrift implementations)
categories.
+/// These are:
+///
+/// 1. **Transport errors**: errors encountered while writing byes to the
I/O
+///channel. These include "connection closed", "bind errors", etc.
+/// 2. **Protocol errors**: errors encountered when processing a Thrift
message
+///within the thrift library. These include "exceeding size limits",
+///"unsupported protocol versions", etc.
+/// 3. **Application errors**: errors encountered when
Thrift-compiler-generated
+///'application' code encounters conditions that violate the spec.
These
+///include "out-of-order messages", "missing required-field values",
etc.
+///This category also functions as a catch-all: any error returned by
+///handler functions is automatically encoded as an `ApplicationError`
and
+///returned to the caller.
+/// 4. **User errors**: exception structs defined within the Thrift IDL.
+///
+/// With the exception of `Error::User`, each error variant encapsulates a
+/// corresponding struct which encodes two required fields:
+///
+/// 1. kind: an enum indicating what kind of error was encountered
+/// 2. message: string with human-readable error information
+///
+/// These two fields are encoded and sent over the wire to the remote.
`kind`
+/// is defined by convention while `message` is freeform. If none of the
+/// enumerated error kinds seem suitable use the catch-all `Unknown`.
+///
+/// # Examples
+///
+/// Creating a `TransportError` explicitly.
+///
+/// Conversions are defined from `thrift::TransportError`,
`thrift::ProtocolError`
+/// and `thrift::ApplicationError` to the corresponding `thrift::Error`
variant
+/// (see `err2` below).
+///
+/// ```
+/// use thrift;
+/// use thrift::{TransportError, TransportErrorKind};
+///
+/// let err0: thrift::Result<()> = Err(
+/// thrift::Error::Transport(
+/// TransportError {
+/// kind: TransportErrorKind::TimedOut,
+/// message: format!("connection to server timed out")
+/// }
+/// )
+/// );
+///
+/// let err1: thrift::Result<()> = Err(
+/// thrift::Error::Transport(
+/// TransportError::new(
+/// TransportErrorKind::TimedOut,
+/// format!("connection to server timed out")
+/// )
+/// )
+/// );
+///
+/// let err2: thrift::Result<()> = Err(
+/// thrift::Error::from(
+/// TransportError::new(
+/// TransportErrorKind::TimedOut,
+/// format!("connection to server timed out")
+/// )
+/// )
+/// );
+///
+/// ```
+///
+/// Creating an arbitrary error from a string
+///
+/// ```
+/// use thrift;
+/// use thrift::{ApplicationError, ApplicationErrorKind};
+///
+/// let err0: thrift::Result<()> = Err(
+/// thrift::Error::from("This is an error")
+
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r97210903
--- Diff: lib/rs/src/errors.rs ---
@@ -0,0 +1,678 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::convert::{From, Into};
+use std::error::Error as StdError;
+use std::fmt::{Debug, Display, Formatter};
+use std::{error, fmt, io, string};
+use try_from::TryFrom;
+
+use ::protocol::{TFieldIdentifier, TInputProtocol, TOutputProtocol,
TStructIdentifier, TType};
+
+// FIXME: should all my error structs impl error::Error as well?
+// FIXME: should all fields in TransportError, ProtocolError and
ApplicationError be optional?
+
+/// Thrift error type.
+///
+/// The error type defined here is used throughout this crate as well as in
+/// all Thrift-compiler-generated Rust code. It falls into one of four
+/// standard (i.e. defined by convention in Thrift implementations)
categories.
+/// These are:
+///
+/// 1. **Transport errors**: errors encountered while writing byes to the
I/O
+///channel. These include "connection closed", "bind errors", etc.
+/// 2. **Protocol errors**: errors encountered when processing a Thrift
message
+///within the thrift library. These include "exceeding size limits",
+///"unsupported protocol versions", etc.
+/// 3. **Application errors**: errors encountered when
Thrift-compiler-generated
+///'application' code encounters conditions that violate the spec.
These
+///include "out-of-order messages", "missing required-field values",
etc.
+///This category also functions as a catch-all: any error returned by
+///handler functions is automatically encoded as an `ApplicationError`
and
+///returned to the caller.
+/// 4. **User errors**: exception structs defined within the Thrift IDL.
+///
+/// With the exception of `Error::User`, each error variant encapsulates a
+/// corresponding struct which encodes two required fields:
+///
+/// 1. kind: an enum indicating what kind of error was encountered
+/// 2. message: string with human-readable error information
+///
+/// These two fields are encoded and sent over the wire to the remote.
`kind`
+/// is defined by convention while `message` is freeform. If none of the
+/// enumerated error kinds seem suitable use the catch-all `Unknown`.
+///
+/// # Examples
+///
+/// Creating a `TransportError` explicitly.
+///
+/// Conversions are defined from `thrift::TransportError`,
`thrift::ProtocolError`
+/// and `thrift::ApplicationError` to the corresponding `thrift::Error`
variant
+/// (see `err2` below).
+///
+/// ```
+/// use thrift;
+/// use thrift::{TransportError, TransportErrorKind};
+///
+/// let err0: thrift::Result<()> = Err(
+/// thrift::Error::Transport(
+/// TransportError {
+/// kind: TransportErrorKind::TimedOut,
+/// message: format!("connection to server timed out")
+/// }
+/// )
+/// );
+///
+/// let err1: thrift::Result<()> = Err(
+/// thrift::Error::Transport(
+/// TransportError::new(
+/// TransportErrorKind::TimedOut,
+/// format!("connection to server timed out")
+/// )
+/// )
+/// );
+///
+/// let err2: thrift::Result<()> = Err(
+/// thrift::Error::from(
+/// TransportError::new(
+/// TransportErrorKind::TimedOut,
+/// format!("connection to server timed out")
+/// )
+/// )
+/// );
+///
+/// ```
+///
+/// Creating an arbitrary error from a string
+///
+/// ```
+/// use thrift;
+/// use thrift::{ApplicationError, ApplicationErrorKind};
+///
+/// let err0: thrift::Result<()> = Err(
+/// thrift::Error::from("This is an error")
+
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 Updated: - Auto-generate constructors for all public structs - Auto-generate `Default` impl for structs with all-optional fields - Add constructors for protocol/transport factories - Use constructors/defaults where appropriate in test/tutorial, etc. - Fix `make clean` bugs in Rust makefiles - Clean up some generator code --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 Any thoughts on this? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 I've now added the tutorial. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 @Jens-G Sorry to bother you, but all the Appveyor builds seem stalled. It appears to be holding up the Travis fix #1158 that was approved yesterday as well :/ --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 @anatol No worries! I was glad to do it :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 @anatol My apologies for the earlier spam. FWIW, the latest version of the PR has the following: - The "thrift" crate name (I do not use "rift" anywhere) - I use the "_" suffix instead of the prefixes I used earlier I am just about to submit another change that removes the ".erl" gitignores. I would be very happy to be the thrift library maintainer for rust. There are additional server types I want to add, including one for Tokio when it stabilizes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on a diff in the pull request: https://github.com/apache/thrift/pull/1147#discussion_r96126537 --- Diff: .gitignore --- @@ -206,6 +206,7 @@ project.lock.json /lib/delphi/test/typeregistry/*.local /lib/delphi/test/typeregistry/*.dcu /lib/erl/.generated +/lib/erl/.rebar --- End diff -- I'll remove this. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r96126534
--- Diff: compiler/cpp/src/thrift/generate/t_rs_generator.cc ---
@@ -2722,7 +2756,30 @@ string t_rs_generator::rust_namespace(t_service*
tservice) {
}
string t_rs_generator::rust_struct_name(t_struct* tstruct) {
- return rust_camel_case(tstruct->get_name());
+ string base_struct_name(rust_camel_case(tstruct->get_name()));
+ if (is_reserved(base_struct_name)) {
+return "Res" + base_struct_name;
+ } else {
+return base_struct_name;
+ }
+}
+
+string t_rs_generator::rust_field_name(t_field* tfield) {
+ string base_field_name(rust_snake_case(tfield->get_name()));
+ if (is_reserved(base_field_name)) {
+return "res_" + base_field_name;
--- End diff --
I agree. I've changed this as well - perhaps you were looking at an older
version of the PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on a diff in the pull request: https://github.com/apache/thrift/pull/1147#discussion_r96126542 --- Diff: .gitignore --- @@ -281,6 +293,7 @@ project.lock.json /test/log/ /test/test.log /test/erl/.generated +/test/erl/.rebar --- End diff -- Will do so. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on a diff in the pull request: https://github.com/apache/thrift/pull/1147#discussion_r96126529 --- Diff: lib/rs/Cargo.toml --- @@ -0,0 +1,17 @@ +[package] +name = "rift" --- End diff -- @anatol I've previously changed this - perhaps you accidentally looked at an older version of the PR? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 [puzzled] I removed the reserved prefix and added an underscore suffix as you requested. Perhaps I didn't commit that. Let me check. Sorry about that! And, BTW - I would be very happy to maintain the Rust thrift library and cargo. I have additional changes I'd like to make, including a Tokio implementation. On Sat, Jan 14, 2017 at 3:24 PM -0500, "Allen George" <allen.geo...@gmail.com> wrote: Sorry @anatol - I thought I'd changed the package name everywhere! I must have missed that. Let me fix it ASAP. Sorry! On Sat, Jan 14, 2017 at 3:20 PM -0500, "Anatol Pomozov" <notificati...@github.com> wrote: This patch looks really good. Thank you @allengeorge for your work! Thrift team, is there anything left that prevents rust library from merging? Is there a such status as 'thrift library maintainer'. It would be great if @allengeorge keep maintaining rust library and thrift cargo. â You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 Sorry @anatol - I thought I'd changed the package name everywhere! I must have missed that. Let me fix it ASAP. Sorry! On Sat, Jan 14, 2017 at 3:20 PM -0500, "Anatol Pomozov" <notificati...@github.com> wrote: This patch looks really good. Thank you @allengeorge for your work! Thrift team, is there anything left that prevents rust library from merging? Is there a such status as 'thrift library maintainer'. It would be great if @allengeorge keep maintaining rust library and thrift cargo. â You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 Hi guys - any comments on this PR? I think I've addressed all review comments. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 @Jens-G Could you add `lib/rs/README.md` to the list of files for which a version has to be updated? I will also look and see if I can automate this somehow. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 I've now updated the PR with `TMultiplexedProcessor`, which was the last functional piece missing. I'm now fleshing out the remaining tests as well as documentation and examples. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 @Jens-G Do you know why the Debian steps in Travis are failing with: ``` The command "docker run --net=host -e BUILD_LIBS="$BUILD_LIBS" $BUILD_ENV -v $(pwd):/thrift/src -it thrift-build:$DISTRO build/docker/scripts/$SCRIPT $BUILD_ARG" exited with 2.``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 As for crate ownership, I'm happy to publish (and thereby reserve `thrift`), and then transfer ownership to whoever will be in charge of pushing published crates. The relevant documentation is here: http://doc.crates.io/crates-io.html#cargo-owner Note: I've not done anything yet. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 OK. I believe I've addressed all outstanding comments. @anatol: * Reserved type names now have a `_` suffix. I've confirmed that this does not trip warnings for either snake-cased or camel-cased identifiers * Ran `rustfmt` with default settings on the code * Rebased onto master and squashed all the changes into a single commit @Jens-G: * Should the version of the client library be 1.0.0? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 OK. Will do. @anatol Do you know if it's possible for someone on the thrift team to "reserve" the thrift crate on crates.io? I originally named the library `rift` because I didn't want to claim the name until it was decided that this code was reasonable. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 Oh. Interesting. I forgot that Debian doesn't have a Rust package (yet) though it's being worked on IIRC. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 I'm continuing to add tests and update documentation, and I think I've addressed all the initial comments on this PR. Any further thoughts? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 Also, I think the Appveyor build breakage is unrelated? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 Thanks @markerickson-wf - I also saw the commit improving "make clean". --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 @jsgf For structs it's more involved than it appears - perhaps because of the way I've chosen to represent them. Here are some factors that cause issues: 1. I represent `required` fields as bare types (i.e. no `Option` wrapper). This means that if the user doesn't specify them in the IDL const declaration then `rustc` will fail. 2. I'm not sure how to handle a case where a user specifies a single struct field in the IDL const declaration but ignores the others. I *could* conceivably use ".." but (see below) 3. I currently don't implement a `Defaults` trait for structs. Even if I did generate one, I'd only do it for structs with all optional or opt-in-req-out fields --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 @markerickson-wf Thanks Mark. Here's the error I get when I run `make precross`: ``` Resolving dependencies... The pubspec for thrift 0.10.0 from path has version 1.0.0-dev. make[4]: *** [pub-get-client] Error 65 ``` I see the `pubspec.yaml` in `lib/dart` is set to `1.0.0-dev`, but what's weird (to me) is that the `pubspec.yaml` in the `test/dart` directory only has a path dependency, not a version dependency. There must be a lock file with the specific version somewhere that I missed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on a diff in the pull request: https://github.com/apache/thrift/pull/1147#discussion_r94576295 --- Diff: compiler/cpp/src/thrift/generate/t_rs_generator.cc --- @@ -0,0 +1,2846 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include +#include +#include +#include --- End diff -- OK. I'll code up my own string replace. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on a diff in the pull request: https://github.com/apache/thrift/pull/1147#discussion_r94356759 --- Diff: lib/rs/Cargo.toml --- @@ -0,0 +1,17 @@ +[package] +name = "rift" +description = "Rust Thrift library" +version = "0.5.1" +license = "Apache-2.0" +authors = ["Allen George <allen.geo...@gmail.com>"] +readme = "README.md" +documentation = "https://docs.rs/rift/; +exclude = ["Makefile*"] +keywords = ["thrift"] + --- End diff -- I have changed it to "Apache Thrift Developers <dev@thrift.apache.org>". This is what other author tags read, so I assume that's OK. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on a diff in the pull request: https://github.com/apache/thrift/pull/1147#discussion_r94354976 --- Diff: lib/rs/Cargo.toml --- @@ -0,0 +1,17 @@ +[package] +name = "rift" +description = "Rust Thrift library" +version = "0.5.1" +license = "Apache-2.0" +authors = ["Allen George <allen.geo...@gmail.com>"] +readme = "README.md" +documentation = "https://docs.rs/rift/; +exclude = ["Makefile*"] +keywords = ["thrift"] + --- End diff -- No problem. 1. Should the authors be "Thrift developers"? 2. Isn't the license OK? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94354857
--- Diff: test/ThriftTest.thrift ---
@@ -67,7 +67,7 @@ typedef i64 UserId
struct Bonk
{
1: string message,
- 2: i32 type
+ 2: i32 bonkType
}
--- End diff --
@Jens-G I'm sorry if my comment came across as rude. I'll change the code
generator to prefix reserved names with an identifier.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on a diff in the pull request: https://github.com/apache/thrift/pull/1147#discussion_r94331438 --- Diff: lib/rs/Cargo.toml --- @@ -0,0 +1,17 @@ +[package] +name = "rift" +description = "Rust Thrift library" +version = "0.5.1" +license = "Apache-2.0" +authors = ["Allen George <allen.geo...@gmail.com>"] +readme = "README.md" +documentation = "https://docs.rs/rift/; +exclude = ["Makefile*"] +keywords = ["thrift"] + --- End diff -- What should change? And what should I change it to? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94331337
--- Diff: test/csharp/TestServer.cs ---
@@ -273,39 +273,24 @@ public long testTypedef(long thing)
return mapmap;
}
+// Insanity
+// returns:
+// { 1 => { 2 => argument,
+// 3 => argument,
+//},
+// 2 => { 6 => , },
+// }
public Dictionary<long, Dictionary<Numberz, Insanity>>
testInsanity(Insanity argument)
{
testLogDelegate.Invoke("testInsanity()");
-Xtruct hello = new Xtruct();
--- End diff --
Thank you @Jens-G
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1147 I thought that there was no agreed-upon rust codestyle... At least, I think rustfmt's defaults are winding their way through an RFC process right now no? Terminal Musings: http://www.allengeorge.com/ Raft in Java: https://github.com/allengeorge/libraft/ Twitter: https://twitter.com/allenageorge/ On Sun, Jan 1, 2017 at 10:03 PM, Anatol Pomozov <notificati...@github.com> wrote: > >- Non-Rust changes (like C# test fix) should be extracted into >separate change. >- Could you please run 'rustfmt' tool over rust sources to bring the >sources to common codestyle? > > â > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <https://github.com/apache/thrift/pull/1147#issuecomment-269931189>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AAEO9Ip_tSvLC40JKplteG7lE2YGAowbks5rOGjwgaJpZM4LYwAn> > . > --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94287266
--- Diff: test/csharp/TestServer.cs ---
@@ -273,39 +273,24 @@ public long testTypedef(long thing)
return mapmap;
}
+// Insanity
+// returns:
+// { 1 => { 2 => argument,
+// 3 => argument,
+//},
+// 2 => { 6 => , },
+// }
public Dictionary<long, Dictionary<Numberz, Insanity>>
testInsanity(Insanity argument)
{
testLogDelegate.Invoke("testInsanity()");
-Xtruct hello = new Xtruct();
--- End diff --
I had to fix the C# server because it was not handling `testInsanity`
properly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94287260
--- Diff: test/ThriftTest.thrift ---
@@ -115,14 +115,6 @@ struct CrazyNesting {
4: binary binary_field
}
-union SomeUnion {
--- End diff --
I, uhh...am not sure why this got removed. I'll re-add it. Suffice it to
say, the code handles unions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
GitHub user allengeorge opened a pull request: https://github.com/apache/thrift/pull/1147 THRIFT-2945 Add Rust support This is a PR to add Rust support to Thrift. It is based on today's master, and I've verified that Rust server/client successfully communicates with all cross-platform clients and servers (*). I would be happy to accept and incorporate feedback! Not implemented: * Struct/union constants: Honestly, this looks like it's "not possible" (tm) * Multiplexed processor I will be continuing to add documentation, comments and clean up the code in both the C++ generator as well as the Rust client library. (*) With exception of: 1. D: no D compiler installed 2. Lua: need to figure out a way to use `.dylib` when linking against Lua libs 3. Dart: weird problem with pubspec requiring 1.0.0 instead of 1.0.0-dev You can merge this pull request into a Git repository by running: $ git pull https://github.com/allengeorge/thrift thrift-2945-pr Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1147.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1147 commit 79128c3fab9cce7d0040a1528f122c2d982f8c7d Author: Allen George <allen.geo...@gmail.com> Date: 2016-11-02T12:01:08Z THRIFT-2945 Add Rust support --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1146: THRIFT-2945: Add Rust support to Thrift
Github user allengeorge closed the pull request at: https://github.com/apache/thrift/pull/1146 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1146: THRIFT-2945: Add Rust support to Thrift
Github user allengeorge commented on the issue: https://github.com/apache/thrift/pull/1146 I'm closing and trying again to see if the PR is more tractable. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
