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

get_key_references should not subscribe to related accounts (Issue 1472) #1499

Merged
merged 3 commits into from Jan 7, 2019

Conversation

Projects
3 participants
@jmjatlanta
Copy link
Contributor

commented Dec 27, 2018

Fixes #1472

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Dec 31, 2018

IMO limiting this to an arbitrary value is the wrong approach. Either you want the subscriptions, or you don't. (I guess you don't, so perhaps better remove them completely.)

@abitmore

This comment has been minimized.

Copy link
Member

commented Jan 1, 2019

I agree with @pmconrad. Likely clients will call get_full_accounts or similar after calling this.

@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 1, 2019

get_key_references no longer subscribes to associated accounts, although it still subscribes to the passed-in keys.

#1472 mentions limiting the size of final_results. Should that be an arbitrary number? Should it be configurable? Perhaps it should be done under another issue (more discussion needed)?

@jmjatlanta jmjatlanta changed the title limit subscribed accounts to 100 get_key_references should not subscribe to related accounts (Issue 1472) Jan 1, 2019

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2019

IMO you could limit the size of the incoming vector to a fixed value. Making this configurable is part of #782 / #1478.
Adding limits to the output would also require start and limit parameters, which breaks compatibility. IMO the performance impact is limited anyway.

@jmjatlanta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2019

IMO you could limit the size of the incoming vector to a fixed value. Making this configurable is part of #782 / #1478.

I interpret the above as "set the max limit of the incoming vector to 100 items, and ask manikey123 to make the 100 configurable. Do nothing to limit the size of the returned vector." Is that the correct interpretation?

@abitmore abitmore added this to In progress in Feature Release (201902) via automation Jan 5, 2019

@abitmore abitmore added this to the 201901 - Feature Release milestone Jan 5, 2019

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2019

That's my suggestion, yes.

@manikey123 manikey123 referenced this pull request Jan 5, 2019

Closed

Change hard-coded limitations in API's to configurable #782

26 of 36 tasks complete
@pmconrad
Copy link
Contributor

left a comment

Thanks. I think that's sufficient protection. There are easier ways to overload API nodes than this one.

Feature Release (201902) automation moved this from In progress to Reviewer approved Jan 7, 2019

@jmjatlanta jmjatlanta merged commit fc2ab66 into develop Jan 7, 2019

2 of 3 checks passed

ci/dockercloud Your tests are pending in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

Feature Release (201902) automation moved this from Reviewer approved to Done Jan 7, 2019

@jmjatlanta jmjatlanta deleted the jmj_1472 branch Jan 7, 2019

@jmjatlanta jmjatlanta referenced this pull request Jan 7, 2019

Closed

get_key_references performance issue? #1472

2 of 19 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.