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.

Sign up for GitHub

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

Jump to bottom

perf(arns): cache resolution for non-existent name #208

Merged
merged 1 commit into from
Sep 20, 2024
Merged

perf(arns): cache resolution for non-existent name #208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/resolution/composite-arns-resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export class CompositeArNSResolver implements NameResolver {
const cachedResolution: NameResolution = JSON.parse(
cachedResolutionBuffer.toString(),
);
resolution = cachedResolution; // hold on tho this in case we need it
resolution = cachedResolution; // hold on to this in case we need it
if (
cachedResolution !== undefined &&
cachedResolution.resolvedAt !== undefined &&
Expand All @@ -71,7 +71,7 @@ export class CompositeArNSResolver implements NameResolver {
name,
});
const resolution = await resolver.resolve(name);
if (resolution.resolvedId !== undefined) {
if (resolution.resolvedAt !== undefined) {
this.cache.set(name, Buffer.from(JSON.stringify(resolution)));
this.log.info('Resolved name', { name, resolution });
return resolution;
Expand Down
18 changes: 16 additions & 2 deletions src/resolution/on-demand-arns-resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
});
this.networkProcess = networkProcess;
this.ao = ao;
// TODO: use getRecords instead of getArNSRecord
this.aoCircuitBreaker = new CircuitBreaker(
({ name }: { name: string }) => {
return this.networkProcess.getArNSRecord({ name });
Expand All @@ -90,8 +91,21 @@
// find that name in the network process, using the circuit breaker if there are persistent AO issues
const arnsRecord = await this.aoCircuitBreaker.fire({ name: baseName });

if (arnsRecord === undefined || arnsRecord.processId === undefined) {
throw new Error('Invalid name, arns record not found');
if (arnsRecord === undefined) {
throw new Error('Unexpected undefined from CU');
}

if (

Check failure on line 98 in src/resolution/on-demand-arns-resolver.ts

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest) 

Replace `⏎········arnsRecord·===·null·||⏎········arnsRecord.processId·===·undefined⏎······` with `arnsRecord·===·null·||·arnsRecord.processId·===·undefined`
arnsRecord === null ||
arnsRecord.processId === undefined
) {
return {
name,
resolvedId: undefined,
resolvedAt: Date.now(),
ttl: 300,
processId: undefined,
};
}

const processId = arnsRecord.processId;
Expand Down
15 changes: 14 additions & 1 deletion src/types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,15 +596,28 @@ export interface ValidNameResolution {
processId: string;
}

// Name resolved, but is missing
export interface MissingNameResolution {
name: string;
resolvedId: undefined;
resolvedAt: number;
ttl: number;
processId: undefined;
}
Comment on lines +599 to +606
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in comment and suggest using optional properties for better TypeScript practices

  • There's a typo in the comment on line 599: "occured" should be "occurred".
  • Instead of assigning properties to undefined, consider making these properties optional using ?, which is more idiomatic in TypeScript.

Apply this diff to correct the typo and update the interface:

-// Name resolved, but is missing
+// Name resolved, but is missing details
 export interface MissingNameResolution {
   name: string;
-  resolvedId: undefined;
+  resolvedId?: string;
   resolvedAt: number;
   ttl: number;
-  processId: undefined;
+  processId?: string;
 }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Name resolved, but is missing
export interface MissingNameResolution {
name: string;
resolvedId: undefined;
resolvedAt: number;
ttl: number;
processId: undefined;
}
// Name resolved, but is missing details
export interface MissingNameResolution {
name: string;
resolvedId?: string;
resolvedAt: number;
ttl: number;
processId?: string;
}


// An error occured while resolving the name
export interface FailedNameResolution {
name: string;
resolvedId: undefined;
resolvedAt: undefined;
ttl: undefined;
processId: undefined;
}
Comment on lines +608 to 615
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct typo in comment and use optional properties in FailedNameResolution

  • The comment on line 608 has a typo: "occured" should be "occurred".
  • Similar to the previous interface, consider using optional properties instead of assigning undefined to the properties.

Here is the suggested diff:

-// An error occured while resolving the name
+// An error occurred while resolving the name
 export interface FailedNameResolution {
   name: string;
-  resolvedId: undefined;
-  resolvedAt: undefined;
-  ttl: undefined;
-  processId: undefined;
+  resolvedId?: string;
+  resolvedAt?: number;
+  ttl?: number;
+  processId?: string;
 }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// An error occured while resolving the name
export interface FailedNameResolution {
name: string;
resolvedId: undefined;
resolvedAt: undefined;
ttl: undefined;
processId: undefined;
}
// An error occurred while resolving the name
export interface FailedNameResolution {
name: string;
resolvedId?: string;
resolvedAt?: number;
ttl?: number;
processId?: string;
}


type NameResolution = ValidNameResolution | MissingNameResolution;
type NameResolution =
| ValidNameResolution
| MissingNameResolution
| FailedNameResolution;

export interface NameResolver {
resolve(name: string): Promise<NameResolution>;
Expand Down
Loading