Skip to content

Conversation

@bessbd
Copy link
Member

@bessbd bessbd commented Oct 22, 2016

Travis builds occasionally fail with Java heap space error.

This commit sets higher limits to fix the issue mentioned above.

Travis builds occasionally fail with Java heap space error.

This commit sets higher limits to fix the issue mentioned above.
@liorze
Copy link
Contributor

liorze commented Oct 23, 2016

We actually recommend the following setup: export MAVEN_OPTS="-Xms512m -Xmx1024m -XX:PermSize=256m -XX:MaxPermSize=512m". Have you experienced any issues with this configuration?

Do you think we should enable the tests after we have fixed our flaky tests?

@bessbd
Copy link
Member Author

bessbd commented Oct 23, 2016

@liorze : thank you for the comment!

I actually copied it from my local environment. That's what I have been using.
(Btw, it looks like to me we aren't exactly consistent with these options. See DEVNOTES vs. README.md)

Regarding the tests: yes, that's what I think.
I see two options:
We either fix all the flakiness and make sure it all fits into reasonable time limits.
Or, we create different test suites, one for smoke testing and another one which contains all the tests we have. (Because some of them aren't exactly "unit" tests, afaik.)

@liorze
Copy link
Contributor

liorze commented Oct 23, 2016

I'll update the DEVNOTES as part of FLUME-2945, thanks.

Re/ tests, I think we have a few more tests to fix, it looks doable. Let's give it a try and go with plan b if it takes to much time to fix.

@bessbd
Copy link
Member Author

bessbd commented Oct 23, 2016

@liorze : thank you for offering to fix the DEVNOTES. (Although, I'm not sure how this is related to switching to Java 8)

Do you have any other review comments for this change?

@liorze
Copy link
Contributor

liorze commented Oct 23, 2016

It's related because the MaxPermSize option is deprecated in Java 8.

No, LGTM.

@bessbd
Copy link
Member Author

bessbd commented Oct 23, 2016

I think, if we have MAVEN_OPTS = "..." mvn, then we need to have MAVEN_SKIP_RC = 1.

If we are going to use multiple mvn ... commands, then I think it's easier to set it up in one place. (before_install)

The caching is to speed up the builds. ( https://docs.travis-ci.com/user/caching/#Arbitrary-directories )
I can remove it, that's fine.

@mpercy
Copy link
Contributor

mpercy commented Oct 23, 2016

The caching is to speed up the builds.

Thanks for helping me understand. Maybe add a comment about this for people new to Travis?

I think, if we have MAVEN_OPTS = "..." mvn, then we need to have MAVEN_SKIP_RC = 1.

Why? I don't understand the reason

If we are going to use multiple mvn ... commands, then I think it's easier to set it up in one place.

We aren't doing that yet, right? Would we ever do that?

Personally I like to avoid putting stuff in default locations for build scripts in order to avoid unintended side effects either in other concurrent builds or in future builds. I just don't like to "pollute" the environment. However, I'm guessing Travis uses a Docker image or something like that and wipes out the home dir before each build so you can leave cruft in $HOME and it will never affect future builds. Is that right?

@bessbd
Copy link
Member Author

bessbd commented Oct 23, 2016

Thanks for helping me understand. Maybe add a comment about this for people new to Travis?

I did not notice huge speedups, so I'm just going to remove it for now.

Why? I don't understand the reason

According to http://stackoverflow.com/a/35755762/5323166 , if there are any defaults (mavenrc, etc.) set in the FS, MAVEN_OPTS env var is ignored.

Afaik, all Travis-CI build environments are "disposable" (the Docker stuff you have mentioned), so it should be fine to "pollute" the environment, but we can go YAGNI and change it later.

.travis.yml Outdated
- $HOME/.m2
before_install:
- echo "MAVEN_OPTS='-Xms512m -Xmx1024m -XX:PermSize=256m -XX:MaxPermSize=512m'" > ~/.mavenrc
- export MAVEN_SKIP_RC="true"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have evidence that this line is required? If so, can you please add a comment about it?

Copy link
Member Author

@bessbd bessbd Oct 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I remove the MAVEN_SKIP_RC setting, the build fails.

I think, that line is required. I'll include a comment about this.

@mpercy
Copy link
Contributor

mpercy commented Oct 23, 2016

I did not notice huge speedups, so I'm just going to remove it for now.

I wasn't asking to remove it, just for a comment to be added to the yaml file, but either way is OK with me.

Copy link
Contributor

@mpercy mpercy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 looks good, thanks!

@bessbd
Copy link
Member Author

bessbd commented Oct 24, 2016

Thank you, @mpercy

I'm about to commit this

@asfgit asfgit closed this in 4b44dfc Oct 24, 2016
mcsanady pushed a commit to mcsanady/flume that referenced this pull request Mar 13, 2018
Travis builds occasionally fail with Java heap space error.

This commit sets higher limits to fix the issue mentioned above.

Reviewers: Lior Zeno, Mike Percy

This closes apache#75

Change-Id: I4f4a275833ef513eb7aa56ec9660cf29e9212086
waidr pushed a commit to waidr/flume that referenced this pull request Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants