[GitHub] [spark] dongjoon-hyun commented on a change in pull request #26757: [SPARK-30121][Build] Miss default memory for sbt usage

[GitBox <gi...@apache.org>]

dongjoon-hyun commented on a change in pull request #26757: [SPARK-30121][Build] Miss default memory for sbt usage
URL: https://github.com/apache/spark/pull/26757#discussion_r353900036
 
 

 ##########
 File path: build/sbt-launch-lib.bash
 ##########
 @@ -111,11 +112,10 @@ addDebugger () {
 # a ham-fisted attempt to move some memory settings in concert
 # so they need not be dicked around with individually.
 get_mem_opts () {
-  local mem=${1:-2048}
-  local perm=$(( $mem / 4 ))
-  (( $perm > 256 )) || perm=256
-  (( $perm < 4096 )) || perm=4096
-  local codecache=$(( $perm / 2 ))
 
 Review comment:
   Another advice is that you should not piggy-back orthogonal things into your PR.
   I understand your intention, but you doesn't mention at all that you are trying to remove `local perm`.
   Technically, what you needed was only two-line change `declare sbt_default_mem=2048` and `local mem=${1:-$sbt_default_mem}` in this file. 
   
   In general, please start with the minimal PR and grow it when there is a reviewer' request.
   
   I'd like to recommend to to minimize PR.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org