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

fix: ensure file systems with matching roleArns are registered correctly TDE-1268 #1092

Merged
merged 9 commits into from
Oct 16, 2024

Conversation

blacha
Copy link
Member

@blacha blacha commented Oct 1, 2024

Motivation

file systems are only created for unique roleArns so FileSystemCreated event is not fired twice if two configuration objects have the same roleArn,

give a confiugration with two buckets both using roleA on a service that has a default role roleDefault

s3://foo/ - roleA
s3://bar/ - roleA

requests to

fs.read("s3://foo"); // tries roleDefault then uses roleA
fs.read("s3://foo"); // uses cached roleA

depending on the order of reads the default role may be used far too often

fs.read("s3://foo") // tries roleDefault then uses roleA
fs.read("s3://bar") // tries roleDefault then uses roleA
fs.read("s3://bar") // tries roleDefault then uses roleA

after this change

fs.read("s3://foo") // tries roleDefault then uses roleA
fs.read("s3://bar") // tries roleDefault then uses roleA
fs.read("s3://bar") // uses cached roleA

Modification

hook the file system finder when it needs to find a new file system use that file system to register onto fsa

Checklist

If not applicable, provide explanation of why.

  • Tests updated
  • Docs updated
  • Issue linked in Title

@blacha blacha changed the title fix: ensure file systems with matching roleArns are registered correctly fix: ensure file systems with matching roleArns are registered correctly TDE-1268 Oct 1, 2024
@blacha blacha marked this pull request as ready for review October 7, 2024 21:14
@blacha blacha requested review from a team as code owners October 7, 2024 21:14
src/__test__/fs.test.ts Show resolved Hide resolved
src/__test__/fs.test.ts Outdated Show resolved Hide resolved
src/__test__/fs.test.ts Show resolved Hide resolved
src/fs.register.ts Outdated Show resolved Hide resolved
src/fs.register.ts Outdated Show resolved Hide resolved
paulfouquet
paulfouquet previously approved these changes Oct 14, 2024
src/__test__/fs.test.ts Outdated Show resolved Hide resolved
@blacha blacha added this pull request to the merge queue Oct 16, 2024
Merged via the queue into master with commit e004506 Oct 16, 2024
2 checks passed
@blacha blacha deleted the fix/duplicate-role-arn branch October 16, 2024 20:44
@github-actions github-actions bot mentioned this pull request Oct 16, 2024
l0b0 pushed a commit that referenced this pull request Oct 20, 2024
…tly TDE-1268 (#1092)

#### Motivation

file systems are only created for unique roleArns so FileSystemCreated
event is not fired twice if two configuration objects have the same
roleArn,

give a confiugration with two buckets both using `roleA` on a service
that has a default role `roleDefault`
```
s3://foo/ - roleA
s3://bar/ - roleA
```

requests to 

```typescript
fs.read("s3://foo"); // tries roleDefault then uses roleA
fs.read("s3://foo"); // uses cached roleA
```

depending on the order of reads the default role may be used far too
often

```typescript
fs.read("s3://foo") // tries roleDefault then uses roleA
fs.read("s3://bar") // tries roleDefault then uses roleA
fs.read("s3://bar") // tries roleDefault then uses roleA
```

after this change

```typescript
fs.read("s3://foo") // tries roleDefault then uses roleA
fs.read("s3://bar") // tries roleDefault then uses roleA
fs.read("s3://bar") // uses cached roleA
```


#### Modification

hook the file system finder when it needs to find a new file system use
that file system to register onto `fsa`

#### Checklist

_If not applicable, provide explanation of why._

- [ ] Tests updated
- [ ] Docs updated
- [ ] Issue linked in Title
github-merge-queue bot pushed a commit that referenced this pull request Oct 21, 2024
🤖 I have created a release *beep* *boop*
---


## [4.7.0](v4.6.0...v4.7.0)
(2024-10-21)


### Features

* **basemaps:** Add nz-elevation bucket as valid source s3 bucket.
BM-1088 ([#1109](#1109))
([bc09b74](bc09b74))
* expose method to calculate a sheet code from any x,y
([#1110](#1110))
([ffa03ad](ffa03ad))
* Flatten providers into strings per role TDE-1291
([#1108](#1108))
([92af2f9](92af2f9))


### Bug Fixes

* ensure file systems with matching roleArns are registered correctly
TDE-1268 ([#1092](#1092))
([e004506](e004506))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants