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

Fix scan swallow NotFoundError #43

Merged
merged 9 commits into from
Aug 28, 2017
Merged

Fix scan swallow NotFoundError #43

merged 9 commits into from
Aug 28, 2017

Conversation

hellysmile
Copy link
Member

@hellysmile hellysmile commented Aug 23, 2017

  • bumped to 0.3.1
  • fixes scan.scroll_id returns self._scroll_id, fixed related tests
  • removed silent swallow NotFoundError during scrolling, comes when search context do not exists anymore or scroll was explicitly cleared, added corresponding test
  • removed surrogatepass, it was back-ported from upstream, which has reverted it in master
  • refactor docker es runner to bind on random ports, added ability to run it on MacOS ;)
  • test_elastic_default_loop must do not run inside coroutine, otherwise it will find loop anyway
  • fixed hidden es container creation on parametrize

@hellysmile hellysmile requested a review from asvetlov August 23, 2017 17:25
@hellysmile
Copy link
Member Author

will find linux somewhere and fix docker later

@hellysmile
Copy link
Member Author

docker.api.inspect_container(container.id)['NetworkSettings']['IPAddress']

is so tricky, if ports map not to None, it will return docker internal network like docker0

what about just set host to 0.0.0.0 ???

@codecov-io
Copy link

codecov-io commented Aug 23, 2017

Codecov Report

Merging #43 into master will decrease coverage by 0.46%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
- Coverage   98.85%   98.39%   -0.47%     
==========================================
  Files           7        7              
  Lines         436      435       -1     
  Branches       76       76              
==========================================
- Hits          431      428       -3     
- Misses          2        4       +2     
  Partials        3        3
Impacted Files Coverage Δ
aioelasticsearch/helpers.py 98.82% <100%> (-0.02%) ⬇️
aioelasticsearch/transport.py 97.83% <100%> (ø) ⬆️
aioelasticsearch/__init__.py 90.47% <100%> (-9.53%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7eb9763...f51fbaa. Read the comment docs.

@asvetlov
Copy link
Member

I used binding to random local ports and found the solution is not reliable.
Docker container takes time to start, port could be acquired by other process.
As result tests was fail very often.
Running on container on separate IP completely solves the problem.
If it doesn't work on Mac well we could try different docker network settings, e.g. bridge setup.

"5.5.1 by default"))
parser.addoption("--no-pull", action="store_true", default=False,
help="Don't perform docker images pulling")
parser.addoption('--es_tag', action='append', default=[],
Copy link
Member

Choose a reason for hiding this comment

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

Why do you hate double quotes?

def es_container(docker, session_id, es_tag, request):
image = 'docker.elastic.co/elasticsearch/elasticsearch:{}'.format(es_tag)
def es_container(docker, session_id, es_tag, unused_port, request):
image = 'docker.elastic.co/elasticsearch/elasticsearch:{es_tag}'.format(
Copy link
Member

Choose a reason for hiding this comment

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

Formatting for formatting, isn't it?

@hellysmile
Copy link
Member Author

hellysmile commented Aug 24, 2017

Regarding docker for mac

https://docs.docker.com/docker-for-mac/networking/

  • I cannot ping my containers

Unfortunately, due to limitations in macOS, we’re unable to route traffic to containers, and from containers back to the host.`

  • There is no docker0 bridge on macOS

Because of the way networking is implemented in Docker for Mac, you cannot see a docker0 interface in macOS. This interface is actually within HyperKit.

  • Per-container IP addressing is not possible

The docker (Linux) bridge network is not reachable from the macOS host.

As I understand there is no way to bridge configuration...

@hellysmile
Copy link
Member Author

Another problem that it is impossible to combine pytest fixture + scopes due pytest-dev/pytest#519 , right now tests make each time new elasticsearch container

@hellysmile
Copy link
Member Author

Formatting comments were fixed

@hellysmile
Copy link
Member Author

hellysmile commented Aug 24, 2017

Previously tests for test_scan_equal_chunks_for_loop were silently making new container, which was extremely slow and actually hidden.

As well if tests were failing hard, container hasn't been removed

So, container will be removed only on atexit, as well it prevent "hard" to create containers with same session_id

added option --local-docker to run on new port and use 0.0.0.0 as host, as well by default new docker host will be used

@hellysmile hellysmile merged commit c677866 into master Aug 28, 2017
@hellysmile hellysmile deleted the 3_0_1_release branch August 28, 2017 09:56
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