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

Multiple tweaks #139

Merged
merged 5 commits into from
Oct 4, 2022
Merged

Multiple tweaks #139

merged 5 commits into from
Oct 4, 2022

Conversation

hovaesco
Copy link
Member

Overview

Resolves #133

Update docker testing images and scripts commit is to overcome flaky tests issues.

Checklist

  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • README.md updated and added information about my change
  • I have run changie new to create a changelog entry

@github-actions
Copy link

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

docker/init_starburst.bash Outdated Show resolved Hide resolved
exclude:
- engine: "starburst_galaxy"
python: "3.9"
isForkBranch: true
isMaster: false
Copy link
Member

Choose a reason for hiding this comment

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

Why isMaster? Sometimes rebase can cause some changes to only occur once merged with latest master.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, Galaxy tests are not running on master.

Copy link
Member

Choose a reason for hiding this comment

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

It would still trigger on isForkBranch true and isMaster true.

I think just using one condition seems simpler. Note that I changed into isStarburstBranch.

(github.event_name == 'pull_request' && contains(github.event.pull_request.head.repo.full_name, 'starburstdata')) || github.event_name != 'pull_request'

@hovaesco hovaesco force-pushed the hovaesco/multiple-tweaks branch 2 times, most recently from 7e74784 to 4cef134 Compare October 3, 2022 12:52
@findinpath findinpath self-requested a review October 3, 2022 13:00
docker-compose -f docker-compose-trino.yml up -d
docker-compose -f docker/util.yml run --rm util wait_for_up trino 8080
while ! docker-compose -f docker-compose-trino.yml logs trino 2>&1 | tail -n 1 | grep "SERVER STARTED"; do sleep 2; done
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you opt to not have timeout option anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

added timeout

-Xmx1G
-XX:-UseBiasedLocking
-XX:+UseG1GC
-XX:InitialRAMPercentage=80
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably this should go into a commit of its own to have corresponding comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

extracted to the separate commit

@findinpath
Copy link
Collaborator

Side aspect from running against Starburst Galaxy:

https://github.com/starburstdata/dbt-trino/actions/runs/3174859319/jobs/5172178878

  /opt/hostedtoolcache/Python/3.9.14/x64/lib/python3.9/site-packages/urllib3/connectionpool.py:1045: InsecureRequestWarning: Unverified HTTPS request is being made to host '***'. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/1.26.x/advanced-usage.html#ssl-warnings

@hovaesco hovaesco merged commit 57d2f73 into master Oct 4, 2022
@hovaesco hovaesco deleted the hovaesco/multiple-tweaks branch October 4, 2022 09:50
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.

Enable snapshot and merge tests for Galaxy
3 participants