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

Feature/domain service #361

Merged
merged 7 commits into from
Jul 22, 2021
Merged

Feature/domain service #361

merged 7 commits into from
Jul 22, 2021

Conversation

just-at-uber
Copy link
Contributor

@just-at-uber just-at-uber commented Jul 20, 2021

Added

  • DomainService which allows searching for a domain with a partial/complete querystring.

@just-at-uber just-at-uber marked this pull request as ready for review July 21, 2021 21:47
@just-at-uber just-at-uber requested a review from a team July 21, 2021 21:47
Copy link

@demirkayaender demirkayaender left a comment

Choose a reason for hiding this comment

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

Overall looks good. I couldn't quite understand how this is gonna behave during an update as explained in the comment.

Another thing (not a blocker for this change) is to think about how to let user clear the cache when they are searching for a new domain. Something like
"Cache is updated at @timestamp" [Update Now]
would help user and also can avoid confusion if they can't figure out why their domain doesn't show up.

});
});

describe('when nextPageToken is returned', () => {

Choose a reason for hiding this comment

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

do you need a case where the cache is updated and the result set changed during pagination?

Copy link
Contributor Author

@just-at-uber just-at-uber Jul 22, 2021

Choose a reason for hiding this comment

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

I don't think that case can occur in the same request.

so to put another way:

say request1 is using the cached version
following path from

server/router/services/domain-service/index.js

calls getDomainList which asks cacheManager if it needs new request or not to cadence server, it will simply return domainList which is cached.

Then continues to getMatchingDomains based on the querystring the user typed.

now say request2 occurs after cache period expires

calls getDomainList which asks cacheManager if it needs new request or not to cadence server, it will then fetch next page until nextPageToken returns undefined. then returns domain list and calls getMatchingDomains

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cadence-server will continue to paginate even if the result has changed since is my understanding

@just-at-uber just-at-uber merged commit 9e4c44a into master Jul 22, 2021
@just-at-uber just-at-uber deleted the feature/domain-service branch July 22, 2021 21:58
@just-at-uber just-at-uber mentioned this pull request Jul 28, 2021
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.

2 participants