[GitHub] thrift issue #1036: THRIFT-3867 Specify BinaryProtocol and CompactProtocol

[erikvanoosten]

Github user erikvanoosten commented on the issue:

https://github.com/apache/thrift/pull/1036
  
The specification documents are up at 
https://erikvanoosten.github.io/thrift-missing-specification/.


---
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 #1036: THRIFT-3867 Specify BinaryProtocol and CompactProtocol

[erikvanoosten]

Github user erikvanoosten commented on the issue:

https://github.com/apache/thrift/pull/1036
  
The feedback cycle took more then a month. This is too long for me. I do 
not have the mental capacity to keep work in my head for that long.

Kind regards,
 Erik.



Op 20-09-16 om 21:49 schreef Jens Geyer:
>
> Why?
>
> —
> You are receiving this because you modified the open/close state.
> 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 #1036: THRIFT-3867 Specify BinaryProtocol and CompactProtocol

[erikvanoosten]

Github user erikvanoosten commented on the issue:

https://github.com/apache/thrift/pull/1036
  
Feel free to take it. I won't support it anymore though.


---
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 #1036: THRIFT-3867 Specify BinaryProtocol and CompactProtocol

[Jens-G]

Github user Jens-G commented on the issue:

https://github.com/apache/thrift/pull/1036
  
Thanks for the reminder. Actually, I like it. Too bad it's closed 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 #1036: THRIFT-3867 Specify BinaryProtocol and CompactProtocol

[Jens-G]

Github user Jens-G commented on the issue:

https://github.com/apache/thrift/pull/1036
  
Why?


---
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 #1036: THRIFT-3867 Specify BinaryProtocol and CompactProtocol

[erikvanoosten]

Github user erikvanoosten commented on the issue:

https://github.com/apache/thrift/pull/1036
  
Closing this due to lack of interest of repo owners.


---
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 #1036: THRIFT-3867 Specify BinaryProtocol and CompactProtocol

[erikvanoosten]

Github user erikvanoosten commented on the issue:

https://github.com/apache/thrift/pull/1036
  
All, @Jens-G, this documentation is ready for another review. Thanks!


---
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 #1036: THRIFT-3867 Specify BinaryProtocol and CompactProtocol

[erikvanoosten]

Github user erikvanoosten commented on the issue:

https://github.com/apache/thrift/pull/1036
  
Note: last commit is still work in progress.


---
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 #1036: THRIFT-3867 Specify BinaryProtocol and CompactProtocol

[erikvanoosten]

Github user erikvanoosten commented on the issue:

https://github.com/apache/thrift/pull/1036
  
@Jens-G wrote:
> The Thrift compiler prevents the combination of oneway plus throws right 
from the start since March 2003, any types other than void are disallowed since 
THRIFT-2030. So that does not hold.

I looked at it again. `ProcessFunction` in thrift 0.9.3 contains the 
following:

```
try {
  result = getResult(iface, args);
} catch(TException tex) {
  LOGGER.error("Internal error processing " + getMethodName(), tex);
  TApplicationException x = new 
TApplicationException(TApplicationException.INTERNAL_ERROR, 
"Internal error processing " + getMethodName());
  oprot.writeMessageBegin(new TMessage(getMethodName(), 
TMessageType.EXCEPTION, seqid));
  x.write(oprot);
  oprot.writeMessageEnd();
  oprot.getTransport().flush();
  return;
}
```

As you can see when `getResult` throws it is encoded regardless of oneway. 
You may ask when does `getResult` throw? Looking at the generated code, for 
example:

```
  public getById_result getResult(I iface, getById_args args) throws 
org.apache.thrift.TException {
getById_result result = new getById_result();
try {
  result.success = iface.getById(args.userId);
} catch (UserNotFoundException exc1) {
  result.exc1 = exc1;
}
return result;
  }
```

it throws when it can not decode the request, or when the service 
implementation throws any unchecked exception.


---
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 #1036: THRIFT-3867 Specify BinaryProtocol and CompactProtocol

[erikvanoosten]

Github user erikvanoosten commented on the issue:

https://github.com/apache/thrift/pull/1036
  
Thanks @Jens-G . That's all useful feedback. Next week I have time to work 
on it further.

Regarding structure: it took me weeks to collect all this information and 
create the document. At some points I no longer could keep the entire thing in 
my head. Your structure proposal makes sense and clears it for me also :)


---
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 #1036: THRIFT-3867 Specify BinaryProtocol and CompactProtocol

[Jens-G]

Github user Jens-G commented on the issue:

https://github.com/apache/thrift/pull/1036
  
Overall pretty neat. I still have a few complaints.

1. The document repeats a lot of stuff that can be found elsewhere, e.g. in 
the Thrift Whitepaper. There is nothing per se wrong with repeating stuff, but 
from my feeling it makes the spec a bit bloaty. 

2. I think the structure should be revised grossly. No need to panic, I'm 
going to explain what I have in mind. By looking at the bits and bytes the 
reader may miss an important point: The order of data is always the same and 
entitrely Independent from the wire format, because that's the way Thrift 
Protocols work. 

 * At the first level we have messages. They have a certain structure.

 * at the next level, a particular data item has a certain structure: a  
map always consists of two types and a count, plus the elements. That does not 
change between protocols. What changes is the way how a particular protocol 
organizes the data onto the wire.

 * and one level deeper we are at what I mean by revising the structure: I 
would not constantly jump back and forth between protocols to explain structs, 
lists, sets, integers, etc. Instead, there should be one chapter focusing on 
binary, and another one for compact. That makes it much easier 
  a) for the reader who is usually interested in only one protocol at a 
time, and 
  b) for future maintainers to add more protocols, such as JSON.




---
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.
---