Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bad error message when giving -m less than 256M #388

Closed
tfmorris opened this issue Oct 15, 2012 · 11 comments
Closed

bad error message when giving -m less than 256M #388

tfmorris opened this issue Oct 15, 2012 · 11 comments
Assignees
Labels
error handling Improving the ways errors are reported to users Good First Issue Indicates issues suitable for newcomers to design or coding, providing a gentle introduction. help wanted An issue that we would love anyone to help us with imported from old code repo Issue imported from Google Code in 2010 Priority: Low Indicates less critical issues that can be dealt with at a later stage Type: Feature Request Identifies requests for new features or enhancements. These involve proposing new improvements.
Milestone

Comments

@tfmorris
Copy link
Member

tfmorris commented Oct 15, 2012

Original author: jeff.al...@gmail.com (May 22, 2011 22:48:28)

What steps will reproduce the problem?

  1. ./refine -m 128M
  2. Get error "incompatible min and max heap size" or something.

What is the expected output? What do you see instead?

I expected it to just work, even on my little machine. It didn't. I discovered the -m argument. I expected it to work, it didn't. I read the shell script and realized I had to hack it to lower the default 256M to 128M.

What version of Google Refine are you using? r1836

What operating system and browser are you using? Linux, Firefox

Is this problem specific to the type of browser you're using or it happens
in all the browsers you tried? No

Original issue: http://code.google.com/p/google-refine/issues/detail?id=388

@tfmorris
Copy link
Member Author

From dfhu...@gmail.com on May 23, 2011 14:07:51:
Jeff, that error message is produced by the JVM. You're right that it would confuse some users. But I think lowering the memory to below 256M is a really rare case. I'd expect people to use the default of 1024M, and then increase it to handle larger data sets. I'm inclined to close this as "Won't Fix", unless there's any objection.

@tfmorris
Copy link
Member Author

From jeff.al...@gmail.com on May 23, 2011 15:26:02:
I had to lower the memory to even get it to start at all, and there was no useful warning message explaining why it was failing. Here's what would have helped me get started with refine without having to read the shell script and file this bug:

mem=`awk '/MemTotal/ {print $2}' /proc/meminfo
if [ "$mem" -lt 768000 ]; then
echo "Warning: your machine has less than 768 megs of RAM. Starting Refine in low memory mode (-m 256M). You probably won't be able to edit larger projects without out of memory errors."
REFINE_MEMORY=256M
fi

Thanks,
-jeff

PS: It does work ok with less memory, at least to work through tutorial-sized experiments.

@tfmorris
Copy link
Member Author

From jeff.al...@gmail.com on May 23, 2011 15:29:08:
Also, just before running the jvm, there should me a check that REFINE_MEMORY is not below 256M, or else you get the confusing error message.

@tfmorris
Copy link
Member Author

From tfmorris on May 25, 2011 05:25:07:
I agree with David that this is an exceedingly rare use case and would support closing the bug report, but I'll leave it to him to make the final decision.

@tfmorris
Copy link
Member Author

From jeff.al...@gmail.com on May 25, 2011 07:21:36:
I don't think it is rare. Here's how to reproduce it:

a. use a laptop from 2005 with 500 megs of RAM and no swap configured
b. download refine
c. run it
d. it won't work with a strange error message about memory
e. discover -m argument, try with 128M because there's no help suggesting another number
f. get a different error that's also misleading

I REALLY REALLY had to want to run Refine to overcome that out of box experience. Others won't.

Here's a simpler proposed fix: make the help message for -m say, "values less than 256M are not allowed", then add the code to prevent them.

'Nuff said.

@tfmorris
Copy link
Member Author

From PaulMake...@gmail.com on May 25, 2011 10:01:47:
One problem is parsing & detecting memory, in a portable way (OS X, Linux, etc): the JVM supports suffixes like "2G" in the -Xm[sx] parameters etc; OS X doesn't have /proc/meminfo; etc.

One compromise might be -m '' which turns off all the heap settings. There's some argument to say that that's a useful thing in any case. Setting the heap sizes doesn't do what a lot of people expect it to...

Certainly, a comment in the wiki page and usage message seems worthwhile.

@thadguidry thadguidry added Type: Feature Request Identifies requests for new features or enhancements. These involve proposing new improvements. error handling Improving the ways errors are reported to users Good First Issue Indicates issues suitable for newcomers to design or coding, providing a gentle introduction. help wanted An issue that we would love anyone to help us with and removed Type: Bug Issues related to software defects or unexpected behavior, which require resolution. labels Apr 24, 2019
@aakankasha028
Copy link
Contributor

aakankasha028 commented Mar 27, 2020

@thadguidry I would like to take up this issue so as to make the first contribution. I propose the following changes

  1. Update the usage message for -m by adding the line "values less than 256M are not allowed"
  2. While parsing the command line arguments, if the value for -m is less than 256 ( if [ $1 -lt 256 ] ) then, show them the usage and exit (or show them an error message and exit)
  3. Add a comment in the wiki page

@wetneb
Copy link
Sponsor Member

wetneb commented Mar 27, 2020

Sounds like a great plan to me!

aakankasha028 added a commit to aakankasha028/OpenRefine that referenced this issue Mar 28, 2020
@aakankasha028
Copy link
Contributor

@wetneb I have made the changes to the shell script and the PR is at #2489. The electricty went off just as I pushed my changes so had to rebase them all over again and then create the PR, hence the delay between the push and the creation of PR

Please review the same. Post that, I will update the wiki page as well.

@wetneb
Copy link
Sponsor Member

wetneb commented Mar 28, 2020

Ok, I am a bit confused by the result. Could you perhaps re-submit the PR as a single commit? Start a new branch from master, cherry-pick your commit from the old branch, push the new branch and submit it as a PR?

@aakankasha028
Copy link
Contributor

Sure, I'll do that!

@wetneb wetneb closed this as completed in 426c715 Mar 28, 2020
wetneb added a commit that referenced this issue Mar 28, 2020
@wetneb wetneb added this to the 3.4 milestone Mar 28, 2020
wetneb added a commit that referenced this issue Nov 4, 2021
This fix was not so useful: it failed to parse the memory setting
properly. Instead of putting an arbitrary lower bound on the memory
setting we accept, we will instead let -m control both REFINE_MEMORY
and REFINE_MIN_MEMORY.

This reverts commit 426c715.
wetneb added a commit that referenced this issue Nov 4, 2021
If we only let them control the max heap size then we can run into
issues when our initial heap size is higher than the maximum one.

Better fix for #388.
Closes #4265.
wetneb added a commit that referenced this issue Nov 4, 2021
… memory settings.

This is a more reliable fix for #388 (#2490).
wetneb added a commit that referenced this issue Nov 4, 2021
… memory settings.

This is a more reliable fix for #388 (#2490).

Closes #4265.
wetneb added a commit that referenced this issue Nov 7, 2021
* Let the -m (UNIX) and /m (Windows) options steer both the min and max memory settings.

This is a more reliable fix for #388 (#2490).

Closes #4265.

* Do the same on Windows
wetneb added a commit that referenced this issue Nov 7, 2021
* Let the -m (UNIX) and /m (Windows) options steer both the min and max memory settings.

This is a more reliable fix for #388 (#2490).

Closes #4265.

* Do the same on Windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error handling Improving the ways errors are reported to users Good First Issue Indicates issues suitable for newcomers to design or coding, providing a gentle introduction. help wanted An issue that we would love anyone to help us with imported from old code repo Issue imported from Google Code in 2010 Priority: Low Indicates less critical issues that can be dealt with at a later stage Type: Feature Request Identifies requests for new features or enhancements. These involve proposing new improvements.
Projects
None yet
Development

No branches or pull requests

4 participants