You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by jaceklaskowski <gi...@git.apache.org> on 2015/12/22 14:19:46 UTC
[GitHub] spark pull request: [CORE] Refactoring: Use pattern matching and d...
GitHub user jaceklaskowski opened a pull request:
https://github.com/apache/spark/pull/10433
[CORE] Refactoring: Use pattern matching and dedicated method
It should be slightly easier to see what really happens (without questions like what other state a task can be here).
BTW, I was thinking about introducing `TaskState.isFailed` or similar method so it's even clearer that the order of cases in the pattern match matters as the only case left out in `TaskState.isFinished` is `FINISHED`. With such a method, `TaskState.isFinished` would be `TaskState.isFailed` + `FINISHED` state. Perhaps, `TaskState.isCompleted` would be more appropriate.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jaceklaskowski/spark taskschedulerimpl-pattern-matching
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/10433.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #10433
----
commit 734095269660da9b6db454ebe49466ffd5cd1b04
Author: Jacek Laskowski <ja...@japila.pl>
Date: 2015-12-22T13:14:05Z
[CORE] Refactoring: Use pattern matching and dedicated method
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [CORE] Refactoring: Use pattern matching and d...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/10433#issuecomment-166616317
I think the logic is equivalent, yes, but it isn't any shorter or clearer. Yes, the issue is that it looks strange to check if the state is FINISHED and then otherwise check if it's a finished state (which means, a finished state that isn't FINISHED). I don't think this is worth it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [CORE] Refactoring: Use pattern matching and d...
Posted by jaceklaskowski <gi...@git.apache.org>.
Github user jaceklaskowski commented on the pull request:
https://github.com/apache/spark/pull/10433#issuecomment-166628562
Doh, you're *again* inviting me to make more involved changes (which is great, but don't think helps others to contribute). What I'm gonna do is to improve the change with changes with `TaskState`. Would that make sense? Would you approve such a change? I know the code quite well now, and believe that it needs such small changes to ultimately be more open for newcomers. There's simply far too much to learn in Spark Core, and such naming conventions where `isFinished` should've been `isCompleted` instead (following the event `CompleteEvent` sent later) don't make it any easier.
Thanks a lot for helping me find the right way! I'm greatly indebted for the effort.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [CORE] Refactoring: Use pattern matching and d...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/10433#issuecomment-166630101
I'm not suggesting any changes at all. If there were a few places in the code that call for checking for a state that is "finished, but not FINISHED" then I'd say it's worth making a name and method for that and refactoring this and the other code to use it. For this alone, it doesn't seem like there's an improvement.
I'm not arguing against improvements; I'm arguing against things that don't appear to be an improvement.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [CORE] Refactoring: Use pattern matching and d...
Posted by markhamstra <gi...@git.apache.org>.
Github user markhamstra commented on the pull request:
https://github.com/apache/spark/pull/10433#issuecomment-166713847
More substantively, `if...else` generally performs better than pattern matching, and this is a fairly hot code path.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [CORE] Refactoring: Use pattern matching and d...
Posted by jaceklaskowski <gi...@git.apache.org>.
Github user jaceklaskowski commented on the pull request:
https://github.com/apache/spark/pull/10433#issuecomment-166618819
I disagree since Spark core is hard due to what it does and making it harder can hinder contributions. There are just such little things that can change how welcoming the code is. Please reconsider your vote (I spent the time on it already so if it's lost aka "I don't think this is worth it", let's turn it back when the change hits master). You've just agreed that the logic is convoluted.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [CORE] Refactoring: Use pattern matching and d...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/10433#issuecomment-166622804
The question is whether this makes the code more 'welcoming' or not, of course, and I don't think this does. I did not agree that the current logic is convoluted; I'm talking about your change, and agreeing with your assessment that relying on `isFinished` to mean "finished but not FINISHED" is funny.
Of course you don't want to spend time on changes that won't be merged. However, the amount of effort that goes into a change alone doesn't make it a better change. No, it would never be appropriate to just merge it and then think later about whether it was worth it.
A lot of my time and others' is spent reviewing changes that won't be merged, so rejected PRs/JIRAs cost everyone time*. Ideally we'd all know what kinds of change will get buy-in from people who can merge ahead of time; the list at https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark#ContributingtoSpark-PreparingtoContributeCodeChanges was written to help describe what makes that likely. I'd also look for JIRAs opened by committers, which are much more likely to have buy-in.
* Well, they have some value: they're a way of communicating about what won't be accepted and have value if this saves time in the future. I think many of your PRs have been rejected on similar grounds, so I'd also look to you to tailor your choices appropriately, rather than push back further. For example, https://github.com/apache/spark/pull/10432 is also pretty trivial but is also fixes with no downside that I can see. Although it's not the most important thing I'd encourage people to work on, myself, that seems OK. Here the issue is that it seems to do nothing but make the code marginally less clear.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [CORE] Refactoring: Use pattern matching and d...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/10433#issuecomment-166615564
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [CORE] Refactoring: Use pattern matching and d...
Posted by jaceklaskowski <gi...@git.apache.org>.
Github user jaceklaskowski closed the pull request at:
https://github.com/apache/spark/pull/10433
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [CORE] Refactoring: Use pattern matching and d...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/10433#issuecomment-166701682
Your debate aside, I am not sure whether it makes sense to change the current one. It has virtually zero impact on anything, and as a result it is more or less changing it for the sake of changing it, which in general isn't a great idea.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [CORE] Refactoring: Use pattern matching and d...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/10433#issuecomment-166813289
@jaceklaskowski can you close the pr?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org