Skip to content

update internal cached mapping upon graph version change #94

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

Merged
merged 5 commits into from
Nov 10, 2020

Conversation

swilly22
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #94 (07e677f) into master (94e2631) will increase coverage by 0.25%.
The diff coverage is 90.40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
+ Coverage   84.98%   85.24%   +0.25%     
==========================================
  Files           8        9       +1     
  Lines         586      664      +78     
==========================================
+ Hits          498      566      +68     
- Misses         88       98      +10     
Impacted Files Coverage Δ
redisgraph/query_result.py 80.72% <78.37%> (+1.05%) ⬆️
redisgraph/graph.py 88.32% <91.83%> (-6.13%) ⬇️
redisgraph/exceptions.py 100.00% <100.00%> (ø)
test.py 98.90% <100.00%> (+0.27%) ⬆️

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 94e2631...07e677f. Read the comment docs.

Comment on lines 59 to 63
if(graph.version != self.graph_version):
# graph version miss-match, this is an indication that
# the graph schema was modified, sync it.
graph.version = self.graph_version
graph._refresh_schema()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that refresh schema should return the timestamp retrieved from the last procedure reply. The schema can be changed between two procedure calls but you will update only according to the latest procedure reply.

Suggested change
if(graph.version != self.graph_version):
# graph version miss-match, this is an indication that
# the graph schema was modified, sync it.
graph.version = self.graph_version
graph._refresh_schema()
graph_version = self.graph_version
while(graph.version != graph_version):
# graph version miss-match, this is an indication that
# the graph schema was modified, sync it.
graph.version = graph_version
graph_version = graph._refresh_schema()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

taking your comment to the extreme I think that the mapping used in the original compact reply can become obsolete by the time the first procedure call is made, although the client cache might be in sync with the server the mapping between the current result-set and the cache might be complete incorrect, in which case I suggest reissuing the query

# construct query command
# ask for compact result-set format
# specify known version
command = ["GRAPH.QUERY", self.name, query, "--compact", "version", self.version]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use version only when server supports this feature, for older version of RedisGraph this will trigger arity error

Comment on lines +276 to +278
# 1. introduce relationship-type 'R'
# 2. introduce label 'L'
# 3. introduce attribute 'x'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see logic below

return QueryResult(self, response)
except redis.exceptions.ResponseError as e:
if "wrong number of arguments" in str(e):
print ("Note: RedisGraph Python requires server version 2.2.8 or above")
Copy link
Contributor

Choose a reason for hiding this comment

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

for now, this is ok
if we introduce new flags there should be a module version check, so each flag is verified with its version and the query will not run if we have a version mismatch

@swilly22 swilly22 merged commit fadc330 into master Nov 10, 2020
@swilly22 swilly22 deleted the graph-version branch November 10, 2020 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants