add protocol methods to protocol source #17

cgardens · 2023-03-31T18:58:33Z

Here's a first-pass at adding the protocol methods to the protocol repo. I could not find a way to make JSONSchema to work nicely. Definitely open to other ways of expressing this. If we like this approach we can clean it up and move forward.

github-actions · 2023-03-31T18:58:45Z

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "add protocol methods to protocol source". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

jdpgrailsdev · 2023-03-31T19:01:10Z

protocol-models/src/main/resources/airbyte_protocol/protocol_methods.md

+
+**interface of both source and destination**
+```
+spec() -> Stream<AirbyteConnectorSpecification>


Is it worth making these objects links into the protocol.yaml file so it is easy to lookup the AirbyteConnectorSpecification, etc or having an appendix at the bottom with links to them?

jdpgrailsdev

evantahler

I like this, but I think the miss in the current implementation is that you can't determine the actual --flag names from the information provided. Some notes on that below.

evantahler · 2023-03-31T20:15:58Z

protocol-models/src/main/resources/airbyte_protocol/protocol_methods.md

+**source only**
+```
+discover(Config) -> AirbyteCatalog
+read(Config, ConfiguredAirbyteCatalog, State) -> Stream<AirbyteRecordMessage | AirbyteStateMessage | AirbyteControlMessage>


I think this method-style representation is a bit misleading. The flag argument isn't --ConfiguredAirbyteCatalog it is --catalog. We also don't explain if are passing the object itself (e.g. stringified JSON) or a file path.

Perhaps a way to keep this method-like signature could be:

read(config -> File<Config>, catalog -> File<ConfiguredAirbyteCatalog>, state -> File<State>) -> Stream<...>

... but now I'm just making things up.

I think JSONSchema for this might work better for this:

{ "$schema": "http://json-schema.org/draft-07/schema#", "type": "object", "additionalProperties": true // this is important now "required": ["config", "catalog"] // showing that state is optional "arguments": { "config": { "type": "file_path", "$ref": Config.yaml }, "catalog": { "type": "file_path", "$ref": ConfiguredAirbyteCatalog.yaml }, "state": { "type": "file_path", "$ref": State.yaml }, } }

Here's a real READ command for reference:

docker run --rm -v $(pwd)/secrets:/secrets -v $(pwd)/integration_tests:/integration_tests airbyte/source-faker:dev read --config /secrets/config.json --catalog /integration_tests/configured_catalog.json

I do think the JSON schema version is a little clearer, maybe there's a way to designate STDIN/STDOUT parameters from arguments to the method call?

evantahler · 2023-03-31T20:16:41Z

protocol-models/src/main/resources/airbyte_protocol/protocol_methods.md

+
+In addition to the return types mentioned below, all methods can return the following message types: `AirbyteLogMessage | AirbyteTraceMessage`.


👍 I missed this note at first

evantahler · 2023-03-31T20:23:28Z

protocol-models/src/main/resources/airbyte_protocol/protocol_methods.md

+* If an input parameter has no name, then it is passed via STDIN.
+
+In addition to the return types mentioned below, all methods can return the following message types: `AirbyteLogMessage | AirbyteTraceMessage`.
+


A note about additional arguments should be ignored and not validated should be here somewhere

jbfbell · 2023-03-31T20:58:08Z

protocol-models/src/main/resources/airbyte_protocol/protocol_methods.md

+## Method Interfaces
+
+We describe these interfaces in pseudocode for clarity. Clarifications on the pseudocode semantics:
+* Any `Stream~ that is mentioned as input arg, is passed to the docker contained via STDIN. 


I feel like the ~ is throwing me off here, I was expecting it to mean something, maybe just empty brackets or filled with a generic Type would be clearer?

Stream<> Stream<T> Stream<...>

jbfbell · 2023-03-31T21:07:32Z

protocol-models/src/main/resources/airbyte_protocol/protocol_methods.md

+* Any `Stream~ that is mentioned as input arg, is passed to the docker contained via STDIN. 
+* All other parameters are passed in as command line args (e.g. --config <path to config file>). 
+* Each input parameter is described as its type (as defined in airbyte_protocol.yml and the name of the parameter). 
+* If an input parameter has no name, then it is passed via STDIN.


I don't think this holds up, or I am misunderstanding it.

Reading this I expected all signatures to be like

methodName(argName: ArgType) // or methodName(ArgType argName) And then there would be a distinction for args passed via stdin which would only be type

jbfbell · 2023-03-31T21:10:51Z

protocol-models/src/main/resources/airbyte_protocol/protocol_methods.md

+**source only**
+```
+discover(Config) -> AirbyteCatalog
+read(Config, ConfiguredAirbyteCatalog, State) -> Stream<AirbyteRecordMessage | AirbyteStateMessage | AirbyteControlMessage>


I do think the JSON schema version is a little clearer, maybe there's a way to designate STDIN/STDOUT parameters from arguments to the method call?

jbfbell · 2023-03-31T21:26:48Z

protocol-models/src/main/resources/airbyte_protocol/protocol_methods.md

+
+We describe these interfaces in pseudocode for clarity. Clarifications on the pseudocode semantics:
+* Any `Stream~ that is mentioned as input arg, is passed to the docker contained via STDIN. 
+* All other parameters are passed in as command line args (e.g. --config <path to config file>). 


Maybe a controversial opinion - I think trying to represent both the stdin/stdout values and the method parameters in the same step is confusing.

Maybe stating that the method returns an I/O stream and define what that I/O accepts and returns as a secondary step?

FWIW, I also think this is a problem in our other docs describing these methods

cgardens added 2 commits March 31, 2023 11:54

add protocol methods to protocol source

dc4a6fc

clarificaton

b2a1dc0

cgardens requested review from evantahler, jdpgrailsdev and jbfbell March 31, 2023 18:59

jdpgrailsdev reviewed Mar 31, 2023

View reviewed changes

jdpgrailsdev approved these changes Mar 31, 2023

View reviewed changes

evantahler requested changes Mar 31, 2023

View reviewed changes

jbfbell reviewed Mar 31, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add protocol methods to protocol source #17

add protocol methods to protocol source #17

cgardens commented Mar 31, 2023 •

edited

Loading

github-actions bot commented Mar 31, 2023 •

edited

Loading

jdpgrailsdev Mar 31, 2023

jdpgrailsdev left a comment

evantahler left a comment

evantahler Mar 31, 2023

evantahler Mar 31, 2023

evantahler Mar 31, 2023

jbfbell Mar 31, 2023

evantahler Mar 31, 2023

evantahler Mar 31, 2023

jbfbell Mar 31, 2023

jbfbell Mar 31, 2023

jbfbell Mar 31, 2023

jbfbell Mar 31, 2023

jbfbell Mar 31, 2023


		In addition to the return types mentioned below, all methods can return the following message types: `AirbyteLogMessage \| AirbyteTraceMessage`.

		* If an input parameter has no name, then it is passed via STDIN.

		In addition to the return types mentioned below, all methods can return the following message types: `AirbyteLogMessage \| AirbyteTraceMessage`.

add protocol methods to protocol source #17

Are you sure you want to change the base?

add protocol methods to protocol source #17

Conversation

cgardens commented Mar 31, 2023 • edited Loading

github-actions bot commented Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

jdpgrailsdev left a comment

Choose a reason for hiding this comment

evantahler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgardens commented Mar 31, 2023 •

edited

Loading

github-actions bot commented Mar 31, 2023 •

edited

Loading