Skip to content
This repository has been archived by the owner on Mar 12, 2024. It is now read-only.

[EXE-1528] Send jdbcOptions as MergedParameter to driver #10

Merged
merged 9 commits into from
Apr 29, 2023

Conversation

ShaoFuWu
Copy link
Collaborator

@ShaoFuWu ShaoFuWu commented Apr 28, 2023

https://actioniq.atlassian.net/browse/EXE-1528
Didn't send queryGroup to driver, details here

sc.setLogLevel("info")
val redshiftOpts = Map(
  "aws_iam_role" -> "arn:aws:iam::938824509083:role/journeys-redshift-dev-redshift-role",
  "tempDir" -> s"s3a://aiq-1d-expire-dev/redshift/109",
  "driver" -> "com.amazon.redshift.jdbc42.Driver",
  "dbtable" -> "qahybridredshift.rd_dim_item",
  "queryGroup" -> "123456678908",
  "aiq_partner" -> "partnerName",
  "ApplicationName" -> "appName",
  "password" -> "",
  "user" -> "admin",
  "url" -> "jdbc:redshift://test-hybridcompute.938824509083.us-east-1.redshift-serverless.amazonaws.com:5439/dev?"
)
spark.read.format("io.github.spark_redshift_community.spark.redshift").options(redshiftOpts).load().show()

Sending a query from spark-shell and see the query_label `select * from SYS_QUERY_HISTORY where query_label = '123456678908'

Deploy Note

sbt publishM2       
aws s3 sync ~/.m2/repository/io/github/spark-redshift-community/ s3://aiq-artifacts/releases/io/github/spark-redshift-community

@ShaoFuWu ShaoFuWu changed the title fix [EXE-1528] Send jdbcOptions as MergedParameter to driver Apr 28, 2023
@ShaoFuWu ShaoFuWu requested review from MasterDDT and yannistze April 28, 2023 18:13
Comment on lines 227 to 229
mergedParameters.parameters.foreach { case (key, value) =>
log.info(s"key: ${key} value ${value}")
properties.setProperty(key, value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you will log every key value, not just the user, password ones like in the original implementation 🤔 Do you need to be logging them for "Prod" or is this a testing remnant ? I am not sure we should be logging them cause they may contain creds 🤷‍♂️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fuuuuuuck will remove, this is for my testing 🤦, good catch!

Copy link
Collaborator

@yannistze yannistze left a comment

Choose a reason for hiding this comment

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

Lgtm 👍

@ShaoFuWu ShaoFuWu merged commit 3d7888d into spark-redshift-community Apr 29, 2023
@@ -97,7 +97,7 @@ class DefaultSource(
}

def tableExists: Boolean = {
val conn = jdbcWrapper.getConnector(params.jdbcDriver, params.jdbcUrl, params.credentials)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious, did you look into the history of why they ignore all the other params?

Even on latest master, only use these 3 params and ignore the others /~https://github.com/spark-redshift-community/spark-redshift/blob/master/src/main/scala/io/github/spark_redshift_community/spark/redshift/RedshiftRelation.scala#L111

Copy link
Collaborator Author

@ShaoFuWu ShaoFuWu Apr 29, 2023

Choose a reason for hiding this comment

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

Looks like they were fixing the same invalid character 8 years ago like we were last week 😅 , the way they fix is passing user/password as part of the options in this pr

@MasterDDT
Copy link
Collaborator

Nice 👍 , this looks like an easy one you could contribute back to the open source fork?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants