java csv destination #505

cgardens · 2020-10-06T23:32:02Z

What

Fixes issue in the javabase where standard in was not being passed all the way to the integration code.
Removes any knowledge of local mount from the integration.
Add FailureTrackingConsumer that makes it easy to write a destination consumer that passes in whether or not any failures occurred into close.
Adds local csv destination that does not depend on singer (and adheres to our new protocol of outputting a blob).
- This works running just with docker run. I have not tested it against the worker.
  e.g.

cat /Users/charles/code/airbyte/airbyte-integrations/csv-destination/src/main/resources/messages.txt | docker run -i -v /Users/charles/code/airbyte/airbyte-integrations/csv-destination/src/main/resources/:/local_resource -v /tmp:/local airbyte/airbyte-csv-destination:dev write --config /local_resource/config.txt --catalog /local_resource/catalog.txt

Checklist

~~Pass in the local mount path from the dockerfile / .env (instead of assuming it is "/local")~~ handle this in the configuration.
Unit tests for FailureTrackingConsumer
Unit tests for CsvDestination
Integration tests for the integration (maybe in separate PR)

Recommended reading order

FailureTrackingConsumer.java
CsvDestination.java
spec.json
component.ts
the rest

cgardens · 2020-10-08T00:26:53Z

...yte-integrations/base-java/src/main/java/io/airbyte/integrations/base/JavaBaseConstants.java

@@ -36,8 +34,4 @@
  public static String ARGS_CATALOG_DESC = "input path for the catalog";
  public static String ARGS_PATH_DESC = "path to the json-encoded state file";

-  // todo (cgardens) - this mount path should be passed in by the worker and read as an arg or
-  // environment variable by the runner.
-  public static Path LOCAL_MOUNT = Path.of("/local");


removed the need for an integration to know about this.

cgardens · 2020-10-08T00:27:26Z

...ns/csv-destination/src/main/java/io/airbyte/integrations/destination/csv/CsvDestination.java

+
+  // todo (cgardens) - we currently don't leverage discover in our destinations, so skipping
+  // implementing it... for now.
+  @Override


are we okay with this choice? i don't want to write instantly dead code unless we think it will not be dead very soon.

We should probably only support it for sources in bash.sh then. It doesn't seem necessary as a separate operation. Presumably being aware of the contents of the destination is something the sync/write operation needs to know internally without the need to expose it outside of the integration.

i think as we become cooler, it will be something we'll want. if we allow complex mapping of fields in source to fields in destination we will need, but for now. i think it's not helpful.

cgardens · 2020-10-08T00:28:36Z

airbyte-integrations/csv-destination/src/main/resources/spec.json

+        "description": "Path to the directory where csv files will be written. Must start with the local mount \"/local\". Any other directory appended on the end will be placed inside that local mount.",
+        "type": "string",
+        "examples": ["/local"],
+        "pattern": "(^\\/local\\/.*)|(^\\/local$)"


enforce that destination path uses the local mount.

cgardens · 2020-10-08T00:30:33Z

docs/integrations/destinations/local-csv2.md

@@ -0,0 +1,34 @@
+# Local CSV


will replace local-csv.md with this one once we switch to this integration. it would be nice, if we could put this doc in the airbyte-integrations/csv-destination.

cgardens · 2020-10-08T00:37:18Z

...tegrations/base-java/src/main/java/io/airbyte/integrations/base/FailureTrackingConsumer.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public abstract class FailureTrackingConsumer<T> implements DestinationConsumer<T> {


not in love with the name of this class. would love suggestions.

jrhizor · 2020-10-08T16:41:18Z

airbyte-integrations/base-java/javabase.sh

@@ -4,4 +4,4 @@ set -e

 # wrap run script in a script so that we can lazy evaluate the value of APPLICATION. APPLICATION is
 # set by the dockerfile that inherits base-java, so it cannot be evaluated when base-java is built.
-bin/"$APPLICATION" "$@"
+cat <&0 | bin/"$APPLICATION" "$@"


Shouldn't the integration runner be responsible for this?

discussed offline. agreed this was right. also agreed to add a comment explaining what's happening. which i did.

jrhizor · 2020-10-08T16:42:28Z

airbyte-integrations/base/base.sh

@@ -51,6 +51,9 @@ function main() {
    # todo: state should be optional: --state "$STATE_FILE"
    eval "$AIRBYTE_READ_CMD" --config "$CONFIG_FILE" --catalog "$CATALOG_FILE"
    ;;
+  write)


we should pass in an env var for source/dest so only one of read and write are valid at this level and show an error if the wrong one is called.

agreed. let's do this in a separate PR.

airbyte-integrations/csv-destination/src/main/resources/spec.json

docs/integrations/destinations/local-csv2.md

...ns/csv-destination/src/main/java/io/airbyte/integrations/destination/csv/CsvDestination.java

jrhizor · 2020-10-08T16:47:38Z

...ns/csv-destination/src/main/java/io/airbyte/integrations/destination/csv/CsvDestination.java

+
+  // todo (cgardens) - we currently don't leverage discover in our destinations, so skipping
+  // implementing it... for now.
+  @Override


We should probably only support it for sources in bash.sh then. It doesn't seem necessary as a separate operation. Presumably being aware of the contents of the destination is something the sync/write operation needs to know internally without the need to expose it outside of the integration.

jrhizor · 2020-10-08T16:49:19Z

...ns/csv-destination/src/main/java/io/airbyte/integrations/destination/csv/CsvDestination.java

+
+  }
+
+  public static void main(String[] args) throws Exception {


Maybe add a comment here so people know this is for local development and testing, not the actual entrypoint to the integration?

this changed. this is the actual entrypoint of the destination now. this moved because the version where IntegrationRunner was the entrypoint got into reflection and jar hell.

This reverts commit 020e1f8.

Base automatically changed from cgardens/java_base to master October 7, 2020 18:21

cgardens force-pushed the cgardens/java_csv_destination branch 2 times, most recently from 843b954 to 33b88a6 Compare October 7, 2020 22:41

cgardens changed the base branch from master to cgardens/schema_to_catalog October 7, 2020 22:42

Base automatically changed from cgardens/schema_to_catalog to master October 7, 2020 22:57

cgardens added 7 commits October 7, 2020 17:25

mvp of csv destination write

85f3d7e

rename the consumer

8787d7c

clean up

a100b2c

remove local mount from integration

e8cc2b9

update spec

54bf8ef

clean up

121d9b4

add rough docs

1300a65

cgardens force-pushed the cgardens/java_csv_destination branch from 6e9d104 to 1300a65 Compare October 8, 2020 00:26

cgardens commented Oct 8, 2020

View reviewed changes

various clean up

ddf557a

cgardens marked this pull request as ready for review October 8, 2020 00:39

cgardens requested review from jrhizor, michel-tricot and sherifnada October 8, 2020 00:39

cgardens added 5 commits October 7, 2020 17:51

add FailureTrackingConsumerTest

c09b25a

CsvDestinationTest

9a3a1db

clean up

39bc4c7

clean up

6afcb67

add toRuntime

020e1f8

jrhizor approved these changes Oct 8, 2020

View reviewed changes

fix destination

dc81794

cgardens added 3 commits October 8, 2020 12:03

remove fake news comment

1aba6bd

feedback

4addc80

Revert "add toRuntime"

d35e670

This reverts commit 020e1f8.

cgardens merged commit 1bf0de6 into master Oct 8, 2020

cgardens deleted the cgardens/java_csv_destination branch October 8, 2020 20:52

karinakuz added connectors/destinations-files connectors/destination/local-csv labels Jan 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

java csv destination #505

java csv destination #505

cgardens commented Oct 6, 2020 •

edited

Loading

cgardens Oct 8, 2020

cgardens Oct 8, 2020 •

edited

Loading

jrhizor Oct 8, 2020

cgardens Oct 8, 2020

cgardens Oct 8, 2020

cgardens Oct 8, 2020

cgardens Oct 8, 2020

jrhizor Oct 8, 2020

cgardens Oct 8, 2020

jrhizor Oct 8, 2020

cgardens Oct 8, 2020

jrhizor Oct 8, 2020

jrhizor Oct 8, 2020

cgardens Oct 8, 2020

java csv destination #505

java csv destination #505

Conversation

cgardens commented Oct 6, 2020 • edited Loading

What

Checklist

Recommended reading order

Choose a reason for hiding this comment

cgardens Oct 8, 2020 • edited Loading

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

Choose a reason for hiding this comment

cgardens commented Oct 6, 2020 •

edited

Loading

cgardens Oct 8, 2020 •

edited

Loading