Skip to content

[SPARK-8619][Streaming]Don't recover keytab and principal configuration within Streaming checkpoint#7008

Closed
SaintBacchus wants to merge 3 commits into
apache:masterfrom
SaintBacchus:SPARK-8619
Closed

[SPARK-8619][Streaming]Don't recover keytab and principal configuration within Streaming checkpoint#7008
SaintBacchus wants to merge 3 commits into
apache:masterfrom
SaintBacchus:SPARK-8619

Conversation

@SaintBacchus

Copy link
Copy Markdown
Contributor

Client.scala will change these configurations, so this would cause the problem that the Streaming recover logic can't find the local keytab file(since configuration was changed)

      sparkConf.set("spark.yarn.keytab", keytabFileName)
      sparkConf.set("spark.yarn.principal", args.principal)

Problem described at Jira

@SaintBacchus SaintBacchus changed the title [SPARK-8619][Streaming]Don't recover keytab and principal configuration within Streaming chckpoint [SPARK-8619][Streaming]Don't recover keytab and principal configuration within Streaming checkpoint Jun 25, 2015
@SparkQA

SparkQA commented Jun 25, 2015

Copy link
Copy Markdown

Test build #35749 has finished for PR 7008 at commit 0d8f800.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class StreamingLinearAlgorithm(object):
    • class StreamingLogisticRegressionWithSGD(StreamingLinearAlgorithm):
    • case class CountFunction(expr: Expression, base: AggregateExpression) extends AggregateFunction
    • case class CountDistinctFunction(
    • case class ApproxCountDistinctPartitionFunction(
    • case class ApproxCountDistinctMergeFunction(
    • case class Sum(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
    • case class CombineSum(child: Expression) extends AggregateExpression
    • case class SumDistinct(child: Expression)
    • case class CombineSetsAndSum(inputSet: Expression, base: Expression) extends AggregateExpression
    • case class CombineSetsAndSumFunction(
    • case class First(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
    • case class Last(child: Expression) extends PartialAggregate with trees.UnaryNode[Expression]
    • case class PrecisionInfo(precision: Int, scale: Int)

@harishreedharan

Copy link
Copy Markdown
Contributor

You don't really need to reload the principal, but that is not a big deal. This LGTM.

We probably should document that these parameters will not be reloaded from checkpoint, so the conf should still have the correct params (not sure if these params were documented in the first place).

@tgravescs - this looks good to go.

@SaintBacchus

Copy link
Copy Markdown
Contributor Author

@harishreedharan You are right, the principal is all the same.
I considerred them as couple configurations, so I added it into the list.:smile:

@harishreedharan

Copy link
Copy Markdown
Contributor

@tgravescs, @tdas - This might be worth getting into a 1.4.1 if another RC is rolled. We should get this into 1.4.2 either way.

@tgravescs

Copy link
Copy Markdown
Contributor

changes look good to me. I'll let @tdas or other streaming maintainer look at commit this though as I'm not as familiar with this code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you call this propertiesToReload? It's confusing to have reloadConfs here and newReloadConf down there when they're not describing the same thing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also, please add a short comment here to explain why this is necessary.

@SparkQA

SparkQA commented Jun 30, 2015

Copy link
Copy Markdown

Test build #36065 has finished for PR 7008 at commit 9b8e92c.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Jun 30, 2015

Copy link
Copy Markdown

Test build #36080 has finished for PR 7008 at commit d50dbdf.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@andrewor14

Copy link
Copy Markdown
Contributor

OK, LGTM I'll merge this into master and 1.4.

@SparkQA

SparkQA commented Jun 30, 2015

Copy link
Copy Markdown

Test build #36170 has finished for PR 7008 at commit d50dbdf.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@tdas

tdas commented Jun 30, 2015

Copy link
Copy Markdown
Contributor

LGTM! Merging this to master and 1.4. Thanks!

asfgit pushed a commit that referenced this pull request Jun 30, 2015
…tion within Streaming checkpoint

[Client.scala](https://github.com/apache/spark/blob/master/yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala#L786) will change these configurations, so this would cause the problem that the Streaming recover logic can't find the local keytab file(since configuration was changed)
```scala
      sparkConf.set("spark.yarn.keytab", keytabFileName)
      sparkConf.set("spark.yarn.principal", args.principal)
```

Problem described at [Jira](https://issues.apache.org/jira/browse/SPARK-8619)

Author: huangzhaowei <carlmartinmax@gmail.com>

Closes #7008 from SaintBacchus/SPARK-8619 and squashes the following commits:

d50dbdf [huangzhaowei] Delect one blank space
9b8e92c [huangzhaowei] Fix code style and add a short comment.
0d8f800 [huangzhaowei] Don't recover keytab and principal configuration within Streaming checkpoint.

(cherry picked from commit d16a944)
Signed-off-by: Tathagata Das <tathagata.das1565@gmail.com>
@asfgit asfgit closed this in d16a944 Jun 30, 2015
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.

6 participants