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

refactor secondary url caching #163

Merged
merged 11 commits into from
Apr 26, 2022
Merged

refactor secondary url caching #163

merged 11 commits into from
Apr 26, 2022

Conversation

murali-shris
Copy link
Member

- What I did

  • Refactored secondary url cache

- How I did it

- How to verify it

- Description for the changelog

@murali-shris murali-shris requested review from gkc and VJag April 21, 2022 12:16
@murali-shris
Copy link
Member Author

murali-shris commented Apr 21, 2022

TODO:

  • exception handling when entry in cache expires and lookup fails - to be taken up later
  • defaultCacheDuration and refreshCacheNow - temporarily commented.have to include in refactored code - to be taken up later
  • throw exceptions from _findSecondary - DONE

}
}

static String stripAtSignFromAtSign(String atSign) {
Copy link
Member

Choose a reason for hiding this comment

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

I thought we had a utility method already somewhere to do this. I believe this method can be moved to a reusable place.

}

static String getNotFoundExceptionMessage(String atSign) {
return 'Unable to find secondary address for atSign:$atSign';
Copy link
Member

Choose a reason for hiding this comment

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

@murali-shris @kalluriramkumar We have to make the exception messages easily discoverable across the SDK. We will revisit this as part of the exception analysis task.

// .. and close the socket
await socket.flush();
socket.destroy();
throw Exception('AtLookup.findSecondary timed out');
Copy link
Member

Choose a reason for hiding this comment

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

According to our exception hierarchy, I recon this corresponds to TimeoutException. We will revisit this in a different PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

made the change to throw exact exceptions


void main() async {
String rootDomain = 'root.atsign.unit.tests';
int rootPort = 64;
SecondaryAddressFinder mockSecondaryFinder = MockSecondaryFinder();
SecondaryUrlFinder mockSecondaryFinder = MockSecondaryFinder();
Copy link
Member

Choose a reason for hiding this comment

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

Very very nice to see mock in action. Abstracting out external dependency into SecondaryUrlFinder makes this possible. Good work Murali. Yayyyy.

Copy link
Member Author

Choose a reason for hiding this comment

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

the tests were written by Gary as a part of initial checkin

Copy link
Member

@VJag VJag left a comment

Choose a reason for hiding this comment

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

Minor comments. Good job.

@murali-shris murali-shris marked this pull request as ready for review April 26, 2022 06:31
@murali-shris murali-shris merged commit 2fd7d7e into trunk Apr 26, 2022
@gkc gkc deleted the secondary_url_caching branch October 30, 2022 16:33
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.

None yet

2 participants