Skip to content

Commit

Permalink
Separate api for getting tenant gitManager. (#14290)
Browse files Browse the repository at this point in the history
[How contribute to this
repo](https://github.com/microsoft/FluidFramework/blob/main/CONTRIBUTING.md).

[Guidelines for Pull
Requests](https://github.com/microsoft/FluidFramework/wiki/PR-Guidelines#guidelines).

The sections included below are suggestions for what you may want to
include.
Feel free to remove or alter parts of this template that do not offer
value for your specific change.

## Description
A lot of consumers of TenantManager.getTenant() across the Server
codebase need only the GitManager property of the returned ITenant
object. Constructing a tenant GitManager instance requires only the
tenant keys (which are already being cached), while retrieving the
tenant data involves a call to tenant DB. This change refactors out the
tenant GitManager instance construction into a separate API and switches
the callers of TenantManager.getTenant() that consume only the
GitManager property to this new API, so that they do not unnecessarily
incur a tenant DB call.

> A concise description of the changes (bug or feature) and their
impact/motivation.
> If this description is short enough to be used as the title, delete
this section and just use the title.

> For bug fixes, also include specifics of how to reproduce it / confirm
it is fixed.

> If this Pull Request should close/resolve any issues when merged, use
[the special syntax for
that](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword)
here.

## Breaking Changes

> If this introduces a breaking change, please describe the impact and
migration path for existing applications below.
> See
[Breaking-vs-Non-breaking-Changes](https://github.com/microsoft/FluidFramework/wiki/Breaking-vs-Non-breaking-Changes)
for details.
> If there are no breaking changes, delete this section.

## Reviewer Guidance

The review process is outlined on [this wiki
page](https://github.com/microsoft/FluidFramework/wiki/PR-Guidelines#guidelines).

> List any specific things you want to get reviewer opinions on, and
anything a reviewer would need to know to review this PR effectively.
> Things you might want to include:
>
> - Questions about how to properly make automated tests for your
changes.
> -   Questions about design choices you made.
> - Descriptions of how to manually test the changes (and how much of
that you have done).
> -   etc.
>
> If you have any questions in this section, consider making the PR a
draft until all questions have been resolved.
  • Loading branch information
arikt-ms committed Feb 25, 2023
1 parent 5d90f7c commit d368f26
Show file tree
Hide file tree
Showing 9 changed files with 37 additions and 31 deletions.
Expand Up @@ -86,8 +86,7 @@ export class DeliLambdaFactory extends EventEmitter implements IPartitionLambdaF
let document: IDocument;

try {
const tenant = await this.tenantManager.getTenant(tenantId, documentId);
gitManager = tenant.gitManager;
gitManager = await this.tenantManager.getTenantGitManager(tenantId, documentId);

// Lookup the last sequence number stored
// TODO - is this storage specific to the orderer in place? Or can I generalize the output context?
Expand Down
Expand Up @@ -84,8 +84,7 @@ export class ScribeLambdaFactory extends EventEmitter implements IPartitionLambd
LumberEventName.ScribeSessionResult, this.serviceConfiguration);

try {
const tenant = await this.tenantManager.getTenant(tenantId, documentId);
gitManager = tenant.gitManager;
gitManager = await this.tenantManager.getTenantGitManager(tenantId, documentId);

summaryReader = new SummaryReader(tenantId, documentId, gitManager, this.enableWholeSummaryUpload);
[latestSummary, document] = await Promise.all([
Expand Down
Expand Up @@ -63,8 +63,7 @@ describe("Routerlicious", () => {
testKafka = new TestKafka();
testProducer = testKafka.createProducer();
testTenantManager = new TestTenantManager();
const tenant = await testTenantManager.getTenant(testTenantId, testDocumentId);
testGitManager = tenant.gitManager as GitManager;
testGitManager = (await testTenantManager.getTenantGitManager(testTenantId, testDocumentId)) as GitManager;
const createTreeEntry: ICreateTreeEntry[] = [];
const requestBody: ICreateTreeParams = {
tree: createTreeEntry,
Expand Down
Expand Up @@ -79,8 +79,7 @@ export class DeltaService implements IDeltaService {
documentId: string,
from?: number,
to?: number) {
const tenant = await this.tenantManager.getTenant(tenantId, documentId);
const gitManager = tenant.gitManager;
const gitManager = await this.tenantManager.getTenantGitManager(tenantId, documentId);

const existingRef = await gitManager.getRef(encodeURIComponent(documentId));
if (!existingRef) {
Expand Down
5 changes: 5 additions & 0 deletions server/routerlicious/packages/services-core/src/tenant.ts
Expand Up @@ -86,6 +86,11 @@ export interface ITenantManager {
*/
getTenant(tenantId: string, documentId: string): Promise<ITenant>;

/**
* Retrieves GitManager instance for the given tenant
*/
getTenantGitManager(tenantId: string, documentId: string): Promise<IGitManager>;

/**
* Verifies that the given auth token is valid. A rejected promise indicates an invalid token.
*/
Expand Down
18 changes: 7 additions & 11 deletions server/routerlicious/packages/services-shared/src/storage.ts
Expand Up @@ -121,8 +121,7 @@ export class DocumentStorage implements IDocumentStorage {
values: [string, ICommittedProposal][],
enableDiscovery: boolean = false,
): Promise<IDocumentDetails> {
const tenant = await this.tenantManager.getTenant(tenantId, documentId);
const gitManager = tenant.gitManager;
const gitManager = await this.tenantManager.getTenantGitManager(tenantId, documentId);

const lumberjackProperties = {
[BaseTelemetryProperties.tenantId]: tenantId,
Expand Down Expand Up @@ -258,30 +257,28 @@ export class DocumentStorage implements IDocumentStorage {
}

public async getVersions(tenantId: string, documentId: string, count: number): Promise<ICommitDetails[]> {
const tenant = await this.tenantManager.getTenant(tenantId, documentId);
const gitManager = tenant.gitManager;
const gitManager = await this.tenantManager.getTenantGitManager(tenantId, documentId);

return gitManager.getCommits(documentId, count);
}

public async getVersion(tenantId: string, documentId: string, sha: string): Promise<ICommit> {
const tenant = await this.tenantManager.getTenant(tenantId, documentId);
const gitManager = tenant.gitManager;
const gitManager = await this.tenantManager.getTenantGitManager(tenantId, documentId);

return gitManager.getCommit(sha);
}

public async getFullTree(tenantId: string, documentId: string): Promise<{ cache: IGitCache; code: string; }> {
const tenant = await this.tenantManager.getTenant(tenantId, documentId);
const versions = await tenant.gitManager.getCommits(documentId, 1);
const gitManager = await this.tenantManager.getTenantGitManager(tenantId, documentId);
const versions = await gitManager.getCommits(documentId, 1);
if (versions.length === 0) {
return {
cache: { blobs: [], commits: [], refs: { [documentId]: (null as unknown) as string }, trees: [] },
code: (null as unknown) as string,
};
}

const fullTree = await tenant.gitManager.getFullTree(versions[0].sha);
const fullTree = await gitManager.getFullTree(versions[0].sha);

let code: string = (null as unknown) as string;
if (fullTree.quorumValues) {
Expand Down Expand Up @@ -373,8 +370,7 @@ export class DocumentStorage implements IDocumentStorage {
}

private async readFromSummary(tenantId: string, documentId: string): Promise<boolean> {
const tenant = await this.tenantManager.getTenant(tenantId, documentId);
const gitManager = tenant.gitManager;
const gitManager = await this.tenantManager.getTenantGitManager(tenantId, documentId);
const existingRef = await gitManager.getRef(encodeURIComponent(documentId));
if (existingRef) {
// Fetch ops from logTail and insert into deltas collection.
Expand Down
20 changes: 14 additions & 6 deletions server/routerlicious/packages/services/src/tenant.ts
Expand Up @@ -9,6 +9,7 @@ import {
ICredentials,
BasicRestWrapper,
getAuthorizationTokenFromCredentials,
IGitManager,
} from "@fluidframework/server-services-client";
import { generateToken, getCorrelationId } from "@fluidframework/server-services-utils";
import * as core from "@fluidframework/server-services-core";
Expand All @@ -19,7 +20,7 @@ export class Tenant implements core.ITenant {
return this.config.id;
}

public get gitManager(): GitManager {
public get gitManager(): IGitManager {
return this.manager;
}

Expand All @@ -31,7 +32,7 @@ export class Tenant implements core.ITenant {
return this.config.orderer;
}

constructor(private readonly config: core.ITenantConfig, private readonly manager: GitManager) {
constructor(private readonly config: core.ITenantConfig, private readonly manager: IGitManager) {
}
}

Expand All @@ -53,10 +54,18 @@ export class TenantManager implements core.ITenantManager {

public async getTenant(tenantId: string, documentId: string, includeDisabledTenant = false): Promise<core.ITenant> {
const restWrapper = new BasicRestWrapper();
const [details, key] = await Promise.all([
const [details, gitManager] = await Promise.all([
restWrapper.get<core.ITenantConfig>(`${this.endpoint}/api/tenants/${tenantId}`,
{ includeDisabledTenant }),
this.getKey(tenantId, includeDisabledTenant)]);
this.getTenantGitManager(tenantId, documentId, includeDisabledTenant)]);

const tenant = new Tenant(details, gitManager);

return tenant;
}

public async getTenantGitManager(tenantId: string, documentId: string, includeDisabledTenant = false): Promise<IGitManager> {
const key = await this.getKey(tenantId, includeDisabledTenant);

const defaultQueryString = {
token: fromUtf8ToBase64(`${tenantId}`),
Expand Down Expand Up @@ -88,9 +97,8 @@ export class TenantManager implements core.ITenantManager {
false,
tenantRestWrapper);
const gitManager = new GitManager(historian);
const tenant = new Tenant(details, gitManager);

return tenant;
return gitManager;
}

public async verifyToken(tenantId: string, token: string): Promise<void> {
Expand Down
Expand Up @@ -65,8 +65,7 @@ export class TestDocumentStorage implements IDocumentStorage {
values: [string, ICommittedProposal][],
enableDiscovery: boolean = false,
): Promise<IDocumentDetails> {
const tenant = await this.tenantManager.getTenant(tenantId, documentId);
const gitManager = tenant.gitManager;
const gitManager = await this.tenantManager.getTenantGitManager(tenantId, documentId);

const blobsShaCache = new Set<string>();
const handle = await writeSummaryTree(gitManager, summary, blobsShaCache, undefined);
Expand Down Expand Up @@ -180,15 +179,13 @@ export class TestDocumentStorage implements IDocumentStorage {
}

public async getVersions(tenantId: string, documentId: string, count: number): Promise<ICommitDetails[]> {
const tenant = await this.tenantManager.getTenant(tenantId, documentId);
const gitManager = tenant.gitManager;
const gitManager = await this.tenantManager.getTenantGitManager(tenantId, documentId);

return gitManager.getCommits(documentId, count);
}

public async getVersion(tenantId: string, documentId: string, sha: string): Promise<ICommit> {
const tenant = await this.tenantManager.getTenant(tenantId, documentId);
const gitManager = tenant.gitManager;
const gitManager = await this.tenantManager.getTenantGitManager(tenantId, documentId);

return gitManager.getCommit(sha);
}
Expand Down
Expand Up @@ -71,6 +71,10 @@ export class TestTenantManager implements ITenantManager {
return this.tenant;
}

public async getTenantGitManager(tenantId: string, documentId: string): Promise<GitManager> {
return this.tenant.gitManager;
}

public async getKey(tenantId: string): Promise<string> {
return "test";
}
Expand Down

0 comments on commit d368f26

Please sign in to comment.