-
Notifications
You must be signed in to change notification settings - Fork 103
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
feat(x-goog-spanner-request-id): add bases #2211
base: main
Are you sure you want to change the base?
Conversation
This change brings in the bases for x-goog-spanner-request-id by commit AtomicCounter and the various utilities plus some unit tests. Ripped out of PR googleapis#2205 Updates googleapis#2200
Kindly pinging @olavloite @surbhigarg92 to help bring this in as it is carved out of the larger PR #2205 and is isolated away from main code. |
@@ -0,0 +1,327 @@ | |||
/*! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
/*! | |
/** |
const X_GOOG_SPANNER_REQUEST_ID_HEADER = 'x-goog-spanner-request-id'; | ||
|
||
class AtomicCounter { | ||
private backingBuffer: Uint32Array; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private backingBuffer: Uint32Array; | |
private readonly backingBuffer: Uint32Array; |
|
||
constructor(initialValue?: number) { | ||
this.backingBuffer = new Uint32Array( | ||
new SharedArrayBuffer(Uint32Array.BYTES_PER_ELEMENT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not familiar with this in Node.js, but while looking into what this is, I ran into this: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer#security_requirements
Does this affect the usability of this in Node.js? Or is this an issue that is only relevant to code running in a browser?
nthRequest: number, | ||
attempt: number | ||
) { | ||
return `1.${randIdForProcess}.${nthClientId}.${channelId}.${nthRequest}.${attempt}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we add a comment that explains why there is a '1' at the start of this string, e.g.:
// The first digit in this string is the version number of the header type that is used.
|
||
const X_GOOG_REQ_ID_REGEX = /(\d+\.){5}\d+/; | ||
|
||
class XGoogRequestHeaderInterceptor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this correctly, this is a server interceptor, meaning that it will be used for testing, right? Could we then move this to a test file?
const prefixesToIgnore: string[] = that.prefixesToIgnore || []; | ||
for (i = 0; i < prefixesToIgnore.length; i++) { | ||
const prefix = prefixesToIgnore[i]; | ||
// console.log(`prefix: ${prefix}\nmethod: ${method}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove?
|
||
function injectRequestIDIntoHeaders( | ||
headers: {[k: string]: string}, | ||
session: any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make this typed?
@@ -0,0 +1,162 @@ | |||
/*! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
/*! | |
/** |
This change brings in the bases for x-goog-spanner-request-id by commit AtomicCounter and the various utilities plus some unit tests.
Ripped out of PR #2205
Updates #2200