Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/517#issuecomment-34486235
PR #557 does indeed add an automatic style checker which includes checking
line lengths.
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/567#issuecomment-34602635
Added a couple comments on top of @rxin's. Thanks so much for doing this
@ScrapCodes!
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/567#issuecomment-34602966
This patch LGTM then. We should get it in soon (once @rxin is happy) to
avoid merge conflicts.
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/582#issuecomment-34829822
Does json4s-jackson include any dependencies that lift-json doesn't? Does
the versioning match?
It seems that this version of json4s-jackson pul
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/582#issuecomment-34836430
Thanks for looking into it! The situation sounds fine for the next minor
release, and I don't think this patch needs to be included in the next
mainte
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/583#issuecomment-34838201
Good catch! LGTM.
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/583#issuecomment-34838620
Alright, now looks-really-good-to-me :)
This should be included in 0.9.1.
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/583#issuecomment-34842990
Created issue
[SPARK-1080](https://spark-project.atlassian.net/browse/SPARK-1080) to
associate with this PR.
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/583#issuecomment-34843731
Merged into master and cherry-picked into branch-0.9 and branch-0.8.
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/584#issuecomment-34844075
Failure was due to scalastyle failing to run (not even a failure due to a
style violation). Weird.
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/584#issuecomment-34844226
Jenkins, retest this please (can I do this?)
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/584#issuecomment-34845592
Jenkins, retest this please.
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/589#issuecomment-34900982
LGTM
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/568#issuecomment-34905299
Looks good to me, will merge as soon as comments have been addressed.
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/589#issuecomment-34905810
Uh-oh, @rxin did your merge include the second commit?
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/589#issuecomment-34908063
Timely merges may not have been the problem -- the title of this PR only
mentions the change you had already made :)
Please signal if your PR is not
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/586#issuecomment-34918907
Thanks for this! Did a quick pass and left some comments.
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/582#issuecomment-34922638
This looks good to me, only significant change is correcting the thrown
exception. I am somewhat underwhelmed by json4s's documentation. For instance,
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/590#issuecomment-34926093
Thanks, merged!
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/592#issuecomment-34951992
What is jenkins currently running? It is a script similar to this, that
simply wasn't previously in our repo? I'm wondering, for instance, how
scal
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/592#issuecomment-34952134
Gotcha, well, this is great, we can tell people that they can run it
manually before submitting a PR to catch style violations and such. Easier than
telling
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/592#issuecomment-34952267
Thanks, merged into master.
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/598#issuecomment-35033971
Thanks! Merged.
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/601#issuecomment-35108234
Thanks! Merged.
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/599#discussion_r9767096
Right now the only way to specify the spark-shell memory is to use the
now-undocumented SPARK_MEM environment variable. I have raised this issue on
the JIRA
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/599#discussion_r9767634
This is what I am referring to:
https://github.com/CodingCat/incubator-spark/blob/spark_shell_improve/bin/spark-class#L93
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/599#issuecomment-35139397
That is ideal, but as Patrick mentioned in #602, we cannot do this because
users currently depend on the behavior of SPARK_MEM. Thus we must deprecate it
and
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/585#discussion_r9777966
Is this a three space indentation?
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/585#discussion_r9777977
not sure this comment is useful, could be removed
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/600#discussion_r9778178
"cm" is back from when these were ClassManifests. Everywhere else it's
still called that, though, so maybe this is a minor cleanup for anothe
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/600#issuecomment-35207151
CC @pwendell and @JoshRosen
This looks good to me in that it should work in the style elsewhere in this
file, but I have a question for someone more
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/602#discussion_r9778280
typo: using => use
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post y
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/602#issuecomment-35207406
Looks good to me, just fix the little typo and I'll merge it in. (I would
do it myself but I'm not certain how to amend the commit while keeping y
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/602#issuecomment-35208183
Jenkins doesn't like me at the moment, @rxin is trying to get that fixed.
In the meantime, can someone with permissions say "Jenkins, test this pleas
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/600#issuecomment-35208921
To answer my own question, the reason we do this is probably because we
have no choice when integrating with Java. Additionally, use of ClassTag as in
our API
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/602#issuecomment-35211178
Neato, Jenkins actually listened to me. Merged into master, thanks!
If your project is set up for it, you can reply to this email and have your
reply appear on
GitHub user aarondav opened a pull request:
https://github.com/apache/incubator-spark/pull/604
SPARK-1098: Minor cleanup of ClassTag usage in Java API
Our usage of fake ClassTags in this manner is probably not healthy, but I'm
not sure if there's a better solution avail
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/604#discussion_r9780093
Note: I removed the ClassTag for V, as it was not necessary. Also, I
reordered the type parameters to put K in front.
---
If your project is set up for it, you
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/604#discussion_r9780099
ClassTags do not store generic information, so here we are still just
finding Tuple2.
---
If your project is set up for it, you can reply to this email and have
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/604#discussion_r9780111
These changes may actually be problematic, as they are part of a publicly
accessible API. Even removing the specialization of V is not really
backwards
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/603#discussion_r9780128
I don't know much about log4j, but could this accidentally override Spark's
own logging properties if it is the first log4j.properties file fo
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/603#issuecomment-35221556
Sorry if this is a stupid question, but why do we need Tachyon JSPs? Are we
going to host Tachyon pages from our own UI?
---
If your project is set up for it
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/599#discussion_r9780175
It is perhaps unfortunate that SPARK_WORKER_MEMORY actually does exist, and
controls the total amount of memory that a worker can lease across all
executors on a
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/599#discussion_r9780203
A slightly confusing point is that the driver is in the same memory space
as the shell, and we're really just controlling the memory of the shell process
i
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/599#discussion_r9780214
Most of the rest of the code in this file uses echo, so I would avoid
changing this unless you have a good reason.
---
If your project is set up for it, you can
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/573#issuecomment-35221813
I'm not 100% caught up on the state of this issue. Is #570 a "complete fix"
for this issue, or is this PR still the best fix we have in the pipe
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/568#issuecomment-35221890
Merged in master and branch-0.9. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/536#discussion_r9780346
Not sure if this is the section you were talking about:
http://kafka.apache.org/documentation.html#kafkahadoopconsumerapi
---
If your project is set up for it
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/536#discussion_r9780371
I don't think a semicolon is gramatically correct here. In fact, I think
"The `updateStateByKey` operation allows you to maintain some state
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/599#discussion_r9781252
nit: maybe "the maximum number of cores to be used by the spark shell"
---
If your project is set up for it, you can reply to this email and have
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/599#discussion_r9781247
update for -em
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post your
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/599#discussion_r9781246
pattern should start with ^ and end with $ -- just tried with something
like "4gz" and it passed
---
If your project is set up for it, you can rep
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/599#discussion_r9781248
change OPTIONS to SPARK_SHELL_OPTS!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/599#discussion_r9781258
update or remove comment at the top of this file that talks about the
options -- removal is fine since we have the list in code now
---
If your project is set
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/600#issuecomment-35225376
This PR doesn't need to block on this discussion, since it doesn't actually
rely on the fake ClassTag, so I've merged it into master.
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/597#issuecomment-35226013
What is the use-case you have in mind here? Just some sort of final status
of all executors right before terminating a job/shell?
If you're
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/600#issuecomment-35226744
No need for your renaming PR @punya -- I've already taken care of it in
#604 :) Thanks!
---
If your project is set up for it, you can reply to this emai
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/604#discussion_r9781736
Went ahead and made these private[spark]. It seems unlikely that anyone
else would use these methods, and making them private means that the type
parameter
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/599#issuecomment-35226986
Jenkins, test this please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/605#issuecomment-35228095
Moving these to src/main/java is a good idea, but I wonder if most of these
files would be better refactored into Scala. This should be trivial for all
except
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/403#issuecomment-35236003
Jenkins, test this please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/475#issuecomment-35236088
Maybe Jenkins was down last time. Jenkins, test this please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/606#discussion_r9798640
Did you add this class? I don't see it in this PR.
also, nit: the previous style was more correct, putting this parameter on
the next line (and both ind
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/605#issuecomment-35305983
Jenkins whitelist is separate from Jenkins admins, the latter of which is
explicitly managed by the people who run the AMPLab Jenkins, alas.
Jenkins
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/403#issuecomment-35307150
Jenkins, add to whitelist and ok to test. (c'mon Jenkins, I believe in you)
---
If your project is set up for it, you can reply to this email and have
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/607#issuecomment-35307506
Jenkins, test this please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/608#issuecomment-35307471
Thanks!! Merged in master and branch-0.9. We should've listened to the
"suspicious shadowing" warning in the first place, no good comes of those
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/572#discussion_r9801137
(1 to folds) is preferred, your style is fine though we use 2 space wrapped
indents instead of 4. Would this be possible, though?
```
(1 to folds).map
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/536#issuecomment-35312964
Thanks! Merged into master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/599#issuecomment-35331962
Thanks! Merged into master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/611#discussion_r9815215
explicit return type
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/611#discussion_r9815294
extra newline?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post your
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/611#discussion_r9815286
Let's not have a default value for this and throw an error if it's not
defined. (I know that wasn't the original behavior, but I think this behavio
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/611#discussion_r9815316
This suggests we may be able to do away with the actor-ness of this class
-- I haven't given it sufficient thought, but I suspect it's the case.
-
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/611#discussion_r9815357
This statement doesn't really give us anything if we're not behind a lock.
I think we should probably synchronize this method and notLeader and
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/611#issuecomment-35361778
Thanks for doing this! Took a very superficial pass, will attempt a deeper
one later. Have you tried running FaultToleranceTest? If you're not on a Linux
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/612#discussion_r9833685
This could be written pretty trivially using an Ordering, but I fear for
the performance characteristics here, since this is a fast path. How does this
look
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/612#issuecomment-35413065
Jenkins, this is ok to test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/612#issuecomment-35413258
Great find here! Thanks very much for submitting this patch. I just had a
suggestion regarding the style of your check. It might be neat if we could add
a test
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/613#discussion_r9833979
Feel free to put them on the same line, we discussed that overly long
imports are perfectly fine.
---
If your project is set up for it, you can reply to this
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/613#discussion_r9834070
braces not required
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/613#discussion_r9834055
These imports of Some are not actually required, just inserted by IDEs
occasionally. This is actually the only remaining usage in our code base, so
let's
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/613#discussion_r9834148
extra whitespace here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/613#discussion_r9834189
nice! Deleted imports make me happy...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/613#issuecomment-35414789
Woo-hoo! Thanks for doing this. This could generate a lot of merge
conflicts, so I will merge after comments are addressed. Since everything
compiles, it&#
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/613#discussion_r9834309
Sorry, just meant there are 2 newlines between the last import and the
following Javadoc. Not a change caused by you, but I thought I'd mention it
a
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/585#discussion_r9834854
This and the one below were changed to private APIs. Also
cogroupResult2ToJava, but that's not here for some reason.
---
If your project is set up for it
GitHub user aarondav opened a pull request:
https://github.com/apache/incubator-spark/pull/615
SPARK-929: Fully deprecate usage of SPARK_MEM
This patch cements our deprecation of the SPARK_MEM environment variable by
replacing it with three more specialized variables
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/613#discussion_r9839626
Nice catch on this one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/605#discussion_r9840846
Hm, let's use JavaSparkContext's fakeClassTag method, so we link to the
documentation on why we're doing this. You may have to rebase to get
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/605#discussion_r9841070
same here
`def returnType(): ClassTag[R] = JavaSparkContext.fakeClassTag`
---
If your project is set up for it, you can reply to this email and have your
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/605#discussion_r9841053
I'm like 85% sure we can use scala.Serializable (which extends
java.io.Serializable) and should not require an import.
---
If your project is set up for it
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/605#issuecomment-35432171
Any reason not to convert StorageLevels as well?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/613#issuecomment-35432325
LGTM too after @rxin's change. Since Double is used throughout that file, I
think renaming JDouble is cleanest.
---
If your project is set up for it, yo
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/615#issuecomment-35433301
I'm amenable to switching to SPARK_DRIVER_MEMORY. I avoided it for two
reasons:
1) Following SPARK_MEM -> SPARK_DRIVER_MEM / SPARK_EXECUTOR_MEM
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/613#issuecomment-35444870
Thanks! Merged into master. This PR was rebased up to the previous master
HEAD, so there are no commits that could've conflicted and result in dupl
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/616#issuecomment-35452922
Thanks! Merged.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post your
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/616#issuecomment-35453423
Also merged into branch-0.9, as we also use scala-2.10.3 there.
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/610#discussion_r9849773
perhaps you meant to add a type here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so
Github user aarondav commented on the pull request:
https://github.com/apache/incubator-spark/pull/610#discussion_r9849854
Unfortuantely, we decided not to go with the return type on a new line
style (see dev list). The acceptable style is one of these two:
```
def
1 - 100 of 135 matches
Mail list logo
