Skip to content

aioredis client with buffer queue #63

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

Closed
wants to merge 8 commits into from
Closed

aioredis client with buffer queue #63

wants to merge 8 commits into from

Conversation

DvirDukhan
Copy link

No description provided.

@codecov
Copy link

codecov bot commented Jun 13, 2021

Codecov Report

Merging #63 (d0f54d5) into master (a3714c1) will increase coverage by 0.69%.
The diff coverage is 92.59%.

❗ Current head d0f54d5 differs from pull request most recent head f1245c5. Consider uploading reports for the commit f1245c5 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
+ Coverage   88.54%   89.24%   +0.69%     
==========================================
  Files           9        9              
  Lines         576      632      +56     
==========================================
+ Hits          510      564      +54     
- Misses         66       68       +2     
Impacted Files Coverage Δ
redisgraph_bulk_loader/label.py 91.93% <66.66%> (ø)
redisgraph_bulk_loader/relation_type.py 87.32% <66.66%> (ø)
redisgraph_bulk_loader/bulk_insert.py 79.62% <78.26%> (-0.18%) ⬇️
redisgraph_bulk_loader/query_buffer.py 98.88% <98.70%> (-1.12%) ⬇️
redisgraph_bulk_loader/config.py 100.00% <100.00%> (ø)
redisgraph_bulk_loader/bulk_update.py 90.74% <0.00%> (+0.08%) ⬆️

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 0660405...f1245c5. Read the comment docs.

Copy link
Contributor

@jeffreylovitz jeffreylovitz left a comment

Choose a reason for hiding this comment

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

This looks good to me, a few comments!

@@ -70,7 +70,8 @@ def process_entities(entities):
@click.option('--max-token-size', '-t', default=500, help='max size of each token in megabytes (default 500, max 512)')
@click.option('--index', '-i', multiple=True, help='Label:Propery on which to create an index')
@click.option('--full-text-index', '-f', multiple=True, help='Label:Propery on which to create an full text search index')
def bulk_insert(graph, host, port, password, user, unix_socket_path, nodes, nodes_with_label, relations, relations_with_type, separator, enforce_schema, skip_invalid_nodes, skip_invalid_edges, escapechar, quote, max_token_count, max_buffer_size, max_token_size, index, full_text_index):
@click.option('--async-requests', '-A', default=3, help='amount of async requests to be executed in parallel' )
Copy link
Contributor

Choose a reason for hiding this comment

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

number instead of amount

requirements.txt Outdated
Comment on lines 2 to 3
click
anyio
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these actually dependencies?

Copy link
Author

Choose a reason for hiding this comment

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

will be removed after rebase

@@ -70,7 +70,8 @@ def process_entities(entities):
@click.option('--max-token-size', '-t', default=500, help='max size of each token in megabytes (default 500, max 512)')
@click.option('--index', '-i', multiple=True, help='Label:Propery on which to create an index')
@click.option('--full-text-index', '-f', multiple=True, help='Label:Propery on which to create an full text search index')
def bulk_insert(graph, host, port, password, user, unix_socket_path, nodes, nodes_with_label, relations, relations_with_type, separator, enforce_schema, skip_invalid_nodes, skip_invalid_edges, escapechar, quote, max_token_count, max_buffer_size, max_token_size, index, full_text_index):
@click.option('--async-requests', '-A', default=3, help='amount of async requests to be executed in parallel' )
async def bulk_insert(graph, host, port, password, user, unix_socket_path, nodes, nodes_with_label, relations, relations_with_type, separator, enforce_schema, skip_invalid_nodes, skip_invalid_edges, escapechar, quote, max_token_count, max_buffer_size, max_token_size, index, full_text_index, async_requests):
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, this function is async because otherwise it cannot make async calls?

Copy link
Author

Choose a reason for hiding this comment

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

correct

@swilly22
Copy link
Contributor

@DvirDukhan please move async_requests into config.py, remove it from:
query_buf = QueryBuffer(graph, client, config, async_requests)

@DvirDukhan DvirDukhan force-pushed the async_client branch 5 times, most recently from 19b5a51 to 740b0f4 Compare June 15, 2021 10:38
@AviAvni AviAvni closed this Sep 14, 2021
@AviAvni
Copy link
Contributor

AviAvni commented Sep 14, 2021

done in #75

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.

4 participants