-
Notifications
You must be signed in to change notification settings - Fork 27
Pick a single search head when search head cluster is detected #150
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
Conversation
}); | ||
}); | ||
export function getSearchJobBySid(service, sid): Promise<any> { | ||
let request = service.getJob(sid); |
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.
All of these refactors were necessary after upgrading the Javascript SDK to version 2+ see: https://github.com/splunk/splunk-sdk-javascript?tab=readme-ov-file#migrate-from-callbacksv1x-to-promiseasync-awaitv2x
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.
In the callback version of the helper code here and in other places there was some error handling that I'm not seeing in the promise version, is this no longer necessary? I.e I would have expected to see some try/catch blocks.
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.
so the resolve/reject handling that I removed is just part of the Promise formation and that's done in the SDK now. For these functions the expectation is that error handling is happening by the caller, which you can see happening here for the SPL2 case for example:
vscode-extension-splunk/out/notebooks/spl2/controller.ts
Lines 37 to 59 in 4bda67d
try { | |
await this.refreshService(); | |
job = await dispatchSpl2Module( | |
this._service, | |
spl2Module, | |
app, | |
subNamespace, | |
earliest, | |
latest, | |
); | |
await super._finishExecution(job, cell, execution); | |
} catch (failedResponse) { | |
let outputItems: vscode.NotebookCellOutputItem[] = []; | |
if (!failedResponse.data || !failedResponse.data.messages) { | |
outputItems = [vscode.NotebookCellOutputItem.error(failedResponse)]; | |
} else { | |
const messages = failedResponse.data.messages; | |
outputItems = splunkMessagesToOutputItems(messages); | |
} | |
execution.replaceOutput([new vscode.NotebookCellOutput(outputItems)]); | |
execution.end(false, Date.now()); | |
} |
sessionKey: service.sessionKey, | ||
authorization: 'Bearer', | ||
}); | ||
newService._originalURL = service._originalURL; |
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.
By keeping the original URL around we can decide whether the user has changed this value or not, if a new value was entered by the user in their settings then that will trigger creation of a new client and re-determining whether that is attached to a search head cluster.
* | ||
* @returns Promise<void> | ||
*/ | ||
export function getSearchHeadClusterMemberClient(service: any): Promise<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.
nit: should the @returns
doctype be Promise<any>
like the return signature?
}); | ||
}); | ||
export function getSearchJobBySid(service, sid): Promise<any> { | ||
let request = service.getJob(sid); |
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.
In the callback version of the helper code here and in other places there was some error handling that I'm not seeing in the promise version, is this no longer necessary? I.e I would have expected to see some try/catch blocks.
const token = config.get<string>('splunk.commands.token'); | ||
// Create a new SDK client if one hasn't been created or token / url settings have been changed | ||
if (!this._service || (this._service._originalURL !== restUrl) || (this._service.sessionKey !== token)) { | ||
this._service = getClient(); |
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.
Should this only be called if this._service
is null?
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.
yeah it would be undefined
but let me fix this to be a bit more specific
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.
done
This avoids any issues with delays associated with sids or other search artifacts being replicated across the cluster. Currently it's possible that a sid created on one search head may not be recognized by another due to delay in artifact replication. This has been tested on a Splunk Cloud cluster of 3 search heads.
Also included: minor refactor of the
splunk.ts
class to adopt Promises used in the Javascript SDK as of version 2.x per https://github.com/splunk/splunk-sdk-javascript?tab=readme-ov-file#migrate-from-callbacksv1x-to-promiseasync-awaitv2x