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

Update query_edge_limit config option info and error message. #2979

Merged
merged 9 commits into from
Feb 15, 2019

Conversation

danielmai
Copy link
Contributor

@danielmai danielmai commented Feb 6, 2019

  • Clarify that the flag --query_edge_limit affects shortest path and recursive queries.
  • Reword query edge limit error since it only counts edges and does not measure memory usage.
  • Remove "ErrToBig" error variable.
  • Add query tests for query edge limit.

This change is Reviewable

Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @danielmai)


query/query3_test.go, line 49 at r1 (raw file):

func TestRecurseEdgeLimitError(t *testing.T) {
	// HACK: Set this flag in the external test cluster when query tests are migrated to

This might not work with the new setup. The current tests are running with the default cluster so it won't be possible to change the edge limit on the fly and setting it to five for all the tests will cause other tests to fail.

One possible solution:

  • Move code in common_test.go to a common package and export all the required functions.
  • Modify existing calls to those functions to point to the new common package.
  • Move these two tests to a different package. TestMain should be identical to the current version.
  • Create a custom cluster in that package with the required changes.

It shouldn't be too difficult. Mostly a lot of refactoring but that should be doable with search and replace. Let me know if you have questions.


query/query3_test.go, line 62 at r1 (raw file):

	ctx := defaultContext()
	_, err := processToFastJsonCtxVars(t, query, ctx, nil)

change this to processQuery


query/query3_test.go, line 294 at r1 (raw file):

	ctx := defaultContext()
	_, err := processToFastJsonCtxVars(t, query, ctx, nil)

change this to processQuery

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: Fix the test and merge.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @danielmai)

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Feel free to transfer to someone if needed, but let's aim to get this in soon.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @danielmai)

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm_cancel:

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @danielmai)

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Not worth writing test for. Also, discard all unnecessary changes.

@martinmr can give the final LGTM.

Reviewable status: 2 of 11 files reviewed, 3 unresolved discussions (waiting on @danielmai and @martinmr)

Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 9 files at r2, 1 of 8 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @danielmai)

@martinmr martinmr merged commit b9ed6b5 into master Feb 15, 2019
@martinmr martinmr deleted the danielmai/query_edge_limit branch February 15, 2019 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants