You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/02/18 05:36:44 UTC
[GitHub] [spark] LuciferYang opened a new pull request #35567: [SPARK-38175][CORE][FOLLOWUP] Clean up unused parameters in private methods signature
LuciferYang opened a new pull request #35567:
URL: https://github.com/apache/spark/pull/35567
### What changes were proposed in this pull request?
This pr is a followup of SPARK-38175 to clean up previously pr omitted parts.
### Why are the changes needed?
Cleanup unused symbol.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Pass GA
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen closed pull request #35567: [SPARK-38175][CORE][FOLLOWUP] Remove `urlPattern` from `HistoryAppStatusStore#replaceLogUrls` method signature
Posted by GitBox <gi...@apache.org>.
srowen closed pull request #35567:
URL: https://github.com/apache/spark/pull/35567
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a change in pull request #35567: [WIP][SPARK-38175][CORE][FOLLOWUP] Clean up unused parameters in private methods signature
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35567:
URL: https://github.com/apache/spark/pull/35567#discussion_r809730677
##########
File path: core/src/main/scala/org/apache/spark/deploy/history/HistoryAppStatusStore.scala
##########
@@ -45,20 +45,20 @@ private[spark] class HistoryAppStatusStore(
override def executorList(activeOnly: Boolean): Seq[v1.ExecutorSummary] = {
val execList = super.executorList(activeOnly)
logUrlPattern match {
- case Some(pattern) => execList.map(replaceLogUrls(_, pattern))
+ case Some(_) => execList.map(replaceLogUrls)
case None => execList
}
}
override def executorSummary(executorId: String): v1.ExecutorSummary = {
val execSummary = super.executorSummary(executorId)
logUrlPattern match {
- case Some(pattern) => replaceLogUrls(execSummary, pattern)
+ case Some(_) => replaceLogUrls(execSummary)
case None => execSummary
}
}
- private def replaceLogUrls(exec: v1.ExecutorSummary, urlPattern: String): v1.ExecutorSummary = {
Review comment:
[SPARK-26311](https://issues.apache.org/jira/browse/SPARK-26311) defines this method with `urlPattern` , and SPARK-26792 change to use `logUrlPattern` held by `ExecutorLogUrlHandler`
##########
File path: core/src/main/scala/org/apache/spark/deploy/history/HistoryAppStatusStore.scala
##########
@@ -45,20 +45,20 @@ private[spark] class HistoryAppStatusStore(
override def executorList(activeOnly: Boolean): Seq[v1.ExecutorSummary] = {
val execList = super.executorList(activeOnly)
logUrlPattern match {
- case Some(pattern) => execList.map(replaceLogUrls(_, pattern))
+ case Some(_) => execList.map(replaceLogUrls)
case None => execList
}
}
override def executorSummary(executorId: String): v1.ExecutorSummary = {
val execSummary = super.executorSummary(executorId)
logUrlPattern match {
- case Some(pattern) => replaceLogUrls(execSummary, pattern)
+ case Some(_) => replaceLogUrls(execSummary)
case None => execSummary
}
}
- private def replaceLogUrls(exec: v1.ExecutorSummary, urlPattern: String): v1.ExecutorSummary = {
Review comment:
[SPARK-26311](https://issues.apache.org/jira/browse/SPARK-26311) defines this method with `urlPattern` and SPARK-26792 change to use `logUrlPattern` held by `ExecutorLogUrlHandler`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #35567: [SPARK-38175][CORE][FOLLOWUP] Remove `urlPattern` from `HistoryAppStatusStore#replaceLogUrls` method signature
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #35567:
URL: https://github.com/apache/spark/pull/35567#issuecomment-1046035966
Merged to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a change in pull request #35567: [SPARK-38175][CORE][FOLLOWUP] Remove `urlPattern` from `HistoryAppStatusStore#replaceLogUrls` method signature
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #35567:
URL: https://github.com/apache/spark/pull/35567#discussion_r809998089
##########
File path: core/src/main/scala/org/apache/spark/deploy/history/HistoryAppStatusStore.scala
##########
@@ -45,20 +45,21 @@ private[spark] class HistoryAppStatusStore(
override def executorList(activeOnly: Boolean): Seq[v1.ExecutorSummary] = {
val execList = super.executorList(activeOnly)
logUrlPattern match {
- case Some(pattern) => execList.map(replaceLogUrls(_, pattern))
+ case Some(_) => execList.map(replaceLogUrls)
Review comment:
Here too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a change in pull request #35567: [SPARK-38175][CORE][FOLLOWUP] Remove `urlPattern` from `HistoryAppStatusStore#replaceLogUrls` method signature
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35567:
URL: https://github.com/apache/spark/pull/35567#discussion_r809977505
##########
File path: core/src/main/scala/org/apache/spark/deploy/history/HistoryAppStatusStore.scala
##########
@@ -45,20 +45,20 @@ private[spark] class HistoryAppStatusStore(
override def executorList(activeOnly: Boolean): Seq[v1.ExecutorSummary] = {
val execList = super.executorList(activeOnly)
logUrlPattern match {
- case Some(pattern) => execList.map(replaceLogUrls(_, pattern))
+ case Some(_) => execList.map(replaceLogUrls)
case None => execList
}
}
override def executorSummary(executorId: String): v1.ExecutorSummary = {
val execSummary = super.executorSummary(executorId)
logUrlPattern match {
- case Some(pattern) => replaceLogUrls(execSummary, pattern)
+ case Some(_) => replaceLogUrls(execSummary)
Review comment:
like [6be9cbf](https://github.com/apache/spark/pull/35567/commits/6be9cbf81a79bbc8babc94a92ac19dad51cc5fd3)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a change in pull request #35567: [SPARK-38175][CORE][FOLLOWUP] Remove `urlPattern` from `HistoryAppStatusStore#replaceLogUrls` method signature
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #35567:
URL: https://github.com/apache/spark/pull/35567#discussion_r809971049
##########
File path: core/src/main/scala/org/apache/spark/deploy/history/HistoryAppStatusStore.scala
##########
@@ -45,20 +45,20 @@ private[spark] class HistoryAppStatusStore(
override def executorList(activeOnly: Boolean): Seq[v1.ExecutorSummary] = {
val execList = super.executorList(activeOnly)
logUrlPattern match {
- case Some(pattern) => execList.map(replaceLogUrls(_, pattern))
+ case Some(_) => execList.map(replaceLogUrls)
case None => execList
}
}
override def executorSummary(executorId: String): v1.ExecutorSummary = {
val execSummary = super.executorSummary(executorId)
logUrlPattern match {
- case Some(pattern) => replaceLogUrls(execSummary, pattern)
+ case Some(_) => replaceLogUrls(execSummary)
Review comment:
This is fine though could simplify to a condition on logUrlPattern.nonEmpty or something
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a change in pull request #35567: [SPARK-38175][CORE][FOLLOWUP] Remove `urlPattern` from `HistoryAppStatusStore#replaceLogUrls` method signature
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35567:
URL: https://github.com/apache/spark/pull/35567#discussion_r810003210
##########
File path: core/src/main/scala/org/apache/spark/deploy/history/HistoryAppStatusStore.scala
##########
@@ -45,20 +45,21 @@ private[spark] class HistoryAppStatusStore(
override def executorList(activeOnly: Boolean): Seq[v1.ExecutorSummary] = {
val execList = super.executorList(activeOnly)
logUrlPattern match {
- case Some(pattern) => execList.map(replaceLogUrls(_, pattern))
+ case Some(_) => execList.map(replaceLogUrls)
Review comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a change in pull request #35567: [SPARK-38175][CORE][FOLLOWUP] Remove `urlPattern` from `HistoryAppStatusStore#replaceLogUrls` method signature
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35567:
URL: https://github.com/apache/spark/pull/35567#discussion_r809999347
##########
File path: core/src/main/scala/org/apache/spark/deploy/history/HistoryAppStatusStore.scala
##########
@@ -45,20 +45,21 @@ private[spark] class HistoryAppStatusStore(
override def executorList(activeOnly: Boolean): Seq[v1.ExecutorSummary] = {
val execList = super.executorList(activeOnly)
logUrlPattern match {
- case Some(pattern) => execList.map(replaceLogUrls(_, pattern))
+ case Some(_) => execList.map(replaceLogUrls)
Review comment:
Sorry, let me fix this too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #35567: [SPARK-38175][CORE][FOLLOWUP] Remove `urlPattern` from `HistoryAppStatusStore#replaceLogUrls` method signature
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35567:
URL: https://github.com/apache/spark/pull/35567#issuecomment-1046043672
thanks @srowen
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a change in pull request #35567: [WIP][SPARK-38175][CORE][FOLLOWUP] Clean up unused parameters in private methods signature
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35567:
URL: https://github.com/apache/spark/pull/35567#discussion_r809730677
##########
File path: core/src/main/scala/org/apache/spark/deploy/history/HistoryAppStatusStore.scala
##########
@@ -45,20 +45,20 @@ private[spark] class HistoryAppStatusStore(
override def executorList(activeOnly: Boolean): Seq[v1.ExecutorSummary] = {
val execList = super.executorList(activeOnly)
logUrlPattern match {
- case Some(pattern) => execList.map(replaceLogUrls(_, pattern))
+ case Some(_) => execList.map(replaceLogUrls)
case None => execList
}
}
override def executorSummary(executorId: String): v1.ExecutorSummary = {
val execSummary = super.executorSummary(executorId)
logUrlPattern match {
- case Some(pattern) => replaceLogUrls(execSummary, pattern)
+ case Some(_) => replaceLogUrls(execSummary)
case None => execSummary
}
}
- private def replaceLogUrls(exec: v1.ExecutorSummary, urlPattern: String): v1.ExecutorSummary = {
Review comment:
[SPARK-26311](https://issues.apache.org/jira/browse/SPARK-26311) defines this method with `urlPattern` and after SPARK-26792, change to use `logUrlPattern` held by `ExecutorLogUrlHandler`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org