Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue 13: Initial Check in of the S3 Connector #19

Merged
merged 65 commits into from
Jul 15, 2021
Merged

Issue 13: Initial Check in of the S3 Connector #19

merged 65 commits into from
Jul 15, 2021

Conversation

karansinghneu
Copy link
Collaborator

@karansinghneu karansinghneu commented May 10, 2021 •
edited by chipmaurer
Loading

This PR fixes Issue #13:

  1. Added all the source code for the standalone presto-s3-connector
  2. Added functionality for JSON Writes and Parquet Reads

@karansinghneu karansinghneu requested review from fpj, adrdc and chipmaurer May 10, 2021 17:05
fpj
fpj requested changes Jun 8, 2021
View reviewed changes
Copy link
Collaborator

@fpj fpj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the license headers, see comment.

gradle.properties Outdated Show resolved Hide resolved
@karansinghneu karansinghneu requested a review from fpj June 8, 2021 22:25
fpj
fpj requested changes Jun 9, 2021
View reviewed changes
Copy link
Collaborator

@fpj fpj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change, @karansinghneu . A couple of other things:

1- Could you make sure that scripts also have the boilerplate notice header in addition to the Java class files? In general, if you are not sure, then put the license header as it causes no harm. Configuration files and generated code don't need it.
2- I see a bunch of /var/log files being included, and I assume that are being included by accident. If I'm right, then can we remove them, please?

gradle.properties Outdated Show resolved Hide resolved
@karansinghneu karansinghneu requested a review from fpj June 9, 2021 16:45
fpj
fpj requested changes Jun 9, 2021
View reviewed changes
Copy link
Collaborator

@fpj fpj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @karansinghneu . A couple more comments.

src/test/bin/minio_start.sh Outdated
@@ -0,0 +1,43 @@
#!/bin/sh

export MINIO_DOCKER_NAME=test-minio
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need these minio scripts, could you explain? Are we copying it from somewhere else? If so, we need to acknowledge the corresponding copyright.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These scripts are written by Chip in order to spin up a minio & schema-registry server to test our connector during development.

src/test/bin/schema_registry_stop.sh Outdated Show resolved Hide resolved
chipmaurer
chipmaurer approved these changes Jun 15, 2021
View reviewed changes
Copy link
Collaborator

@chipmaurer chipmaurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this merged soon!

karansinghneu and others added 12 commits June 16, 2021 11:19
@karansinghneu
Adding modified build file
8924b9e 
Signed-off-by: Karan Singh <[email protected]>
@karansinghneu
Removing log directory
fa86480 
Signed-off-by: Karan Singh <[email protected]>
@chipmaurer
Change integration tests to use scality s3 server
9c1c0c3 
Signed-off-by: Chip Maurer <[email protected]>
@karansinghneu
Adding old gradle build
edb3629 
Signed-off-by: Karan Singh <[email protected]>
@chipmaurer
Merge branch 'presto-karan-s3' of https://github.com/karansinghneu/pr…
02abfba 
…esto-s3-connector into presto-karan-s3
@chipmaurer
Down to 2 failed tests. TestDropTable and TestDropTableJson
c8875da 
Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Set integration test run to include --info flag
3e72564 
Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Use a profile for aws commands. Make sure to pull schema registry bef…
1d88ff9 
…ore run

Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Put docker ps commands in startup to ensure containers are running
1c26dbf 
Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Use curl to test for s3 server readiness
b2f9303 
Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Use Bash for shell
a716810
@chipmaurer
Add some debug and change setup method annotation
3cd7ee0 
Signed-off-by: Chip Maurer <[email protected]>
chipmaurer added 18 commits July 1, 2021 13:55
@chipmaurer
Use localhost instead of 127.0.0.1 - yes, it's dumb but I'm running o…
1ecfaf7 
…ut of ideas

Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Start containers with --bind arg
1783ab0 
Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Remove --bind arg
e24b314 
Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Replace localhost with 127.0.0.1
f444b1a 
Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Back out the yml stuff
01a8531 
Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
add a stupid sleep
506ea6f 
Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Make sure s3_start REALLY finishes
279ed7b 
Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Go back to starting containers in yml file
17735cc 
Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Fail setup on error. Catch listBuckets exception for slow systems dur…
98c9a23 
…ing integration tests

Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
OK, maybe waiting up to 30 seconds isn't enough. Now waiting up to 20…
9025561 
…0 seconds

Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Run docker logs if can't create bucket
b5d58e5 
Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Fix logic errors in setup and s3_start. Add more debug to s3 start sc…
ff3b5e0 
…ript

Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Make sure hmac digest is installed
3b1a31e 
Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Don't use sudo
bbf92fd 
Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Move install of hmac to action file. Replace sh with bash in java set…
b19200e 
…up code

Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Well, I guess sudo doesn't work
49646c8 
Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
So, sudo does work, my bad
d3e1895 
Signed-off-by: Chip Maurer <[email protected]>
@chipmaurer
Updating the README
addf00c 
Signed-off-by: Chip Maurer <[email protected]>
chipmaurer
chipmaurer reviewed Jul 2, 2021
View reviewed changes
build.gradle Outdated Show resolved Hide resolved
fpj
fpj reviewed Jul 7, 2021
View reviewed changes
etc/catalog/s3.properties Outdated Show resolved Hide resolved
etc/config.properties Outdated Show resolved Hide resolved
etc/log.properties Outdated Show resolved Hide resolved
src/main/java/com/facebook/presto/s3/BytesLineReader.java Outdated Show resolved Hide resolved
src/main/java/com/facebook/presto/s3/S3OutputTableHandle.java Outdated Show resolved Hide resolved
src/main/java/com/facebook/presto/s3/S3SplitSource.java Outdated Show resolved Hide resolved
src/main/java/com/facebook/presto/s3/S3TableDescriptionSupplier.java Outdated Show resolved Hide resolved
src/main/java/com/facebook/presto/s3/S3TableDescriptionSupplier.java Outdated Show resolved Hide resolved
src/main/java/com/facebook/presto/s3/decoder/CsvRowDecoder.java Outdated Show resolved Hide resolved
src/main/resources/log4j.properties Outdated Show resolved Hide resolved
@chipmaurer chipmaurer requested a review from fpj July 7, 2021 21:28
fpj
fpj requested changes Jul 14, 2021
View reviewed changes
Copy link
Collaborator

@fpj fpj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karansinghneu could clarify the status of this comment in the PR description:

Requires a fix for the stack corruption of the gradle build (can be fixed temporarily in the S3TableHandle class)

if anything has changed about it, please update the PR description accordingly.

@chipmaurer
Copy link
Collaborator

@fpj - I have updated the PR description. There is no 'fix' for stack corruption needed.

@fpj fpj merged commit c10c872 into EMCECS:main Jul 15, 2021
@chipmaurer chipmaurer mentioned this pull request Jul 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chipmaurer chipmaurer chipmaurer approved these changes

@adrdc adrdc adrdc approved these changes

@fpj fpj fpj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@karansinghneu @chipmaurer @fpj @adrdc