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.

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

[SPARK-LLAP-70]Implement against Apache Spark 2.2 #118

Closed

Conversation

merlintang
Copy link
Contributor

What changes were proposed in this pull request?

(Please fill in changes proposed in this fix)

How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

@merlintang
Copy link
Contributor Author

@bikassaha @dongjoon-hyun you can review this later after I have done the testing.

@dongjoon-hyun
Copy link
Contributor

Thank you for pinging me, @merlingtang. Please let me know after the test pass.

@merlintang merlintang changed the title Branch 2.2 [SPARK-LLAP-70]Implement against Apache Spark 2.2 May 19, 2017
@dongjoon-hyun
Copy link
Contributor

So, the test suite is not passed here for some reasons?

@dongjoon-hyun
Copy link
Contributor

Could you share the test result? Which is passed and failed?

Copy link
Contributor

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Here, the patch file should be against Apache branch-2.2. The current one is a patch for internal branch. We cannot use this here.

+ override def serviceName: String = "hiveserver2"
+
+ override def obtainCredentials(
+ hadoopConf: Configuration,
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation

+ }
+
+ /**
+ * Run some code as the real logged in user (which may differ from the current user, for
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation.

private val HIVE_SESSION_STATE_BUILDER_CLASS_NAME =
"org.apache.spark.sql.hive.HiveSessionStateBuilder"

+ // HDP Llap SessionStateBuilder
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove HDP.


+ // HDP Llap SessionStateBuilder
+ private val LLAP_SESSION_STATE_BUILDER_CLASS_NAME =
+ "org.apache.spark.sql.hive.LlapSessionStateBuilder"
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation.


+
+ /**
+ * Return true if `spark.sql.hive.llap=true` and classes can be loaded.
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation.

/**
- * Run some code as the real logged in user (which may differ from the current user, for
- * example, when using proxying).
- */
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you fix all indentation issues? Especially, this is a wrong update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I will update all of them.

- * Return true if `spark.sql.hive.llap=true` and classes can be loaded.
- * On class loading errors, it will fails.
- * Return false if `spark.sql.hive.llap=false`.
- */
Copy link
Contributor

Choose a reason for hiding this comment

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

Uh... The patch file seems to be wrong.

@dongjoon-hyun
Copy link
Contributor

I finished my first round. @merlintang .

build.sbt Outdated
@@ -1,6 +1,6 @@

name := "spark-llap"
version := "1.1.1-2.1"
version := "1.1.1-2.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to change the versions in assembly together.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you change assembly versions?

@dongjoon-hyun
Copy link
Contributor

Hi, @merlintang .
It seems that it's almost done.
Please change the PR description like #27 .

@dongjoon-hyun
Copy link
Contributor

It's not urgent. Just I left two more comments.

@dongjoon-hyun
Copy link
Contributor

FYI. apache/spark#17710

@merlintang
Copy link
Contributor Author

merlintang commented Jun 28, 2017 via email

@merlintang
Copy link
Contributor Author

I will close this one, since I will open a new PR based on the latest updated branch-2.2.

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

Successfully merging this pull request may close these issues.

2 participants