Conversation
| // MessageRecipient (Source) | ||
| const messageRecipient = $('<MessageRecipient></MessageRecipient>') | ||
| messageRecipient.append($(`<PartyId>${source.toUpperCase()}</PartyId>`)) | ||
| const resolvedRecipient = recipientPartyId || (source === 'sme' ? 'PADPIDA2007040502I' : source.toUpperCase()) |
There was a problem hiding this comment.
I'm not sure what our prototype/roughness comfort level is in this repo. But this seems like it should be a constant and/or some kind of function that can determine the value. Having the source check for a particular label hard-coded in here feels cryptic and hard to maintain :-D
| if (isSuccess) { | ||
| releaseStatus.append($('<ReleaseStatus>SuccessfullyIngestedByReleaseDistributor</ReleaseStatus>')) | ||
| const acknowledgement = $('<Acknowledgement></Acknowledgement>') | ||
| acknowledgement.append($('<MessageType>NewReleaseMessage</MessageType>')) |
There was a problem hiding this comment.
I don't know why it feels wild to me that we're doing all of these lines with string literals for tag names to incrementally build XML. This def feels like something that could eventually be append(Tags.MessageType, Types.NewRelease) etc...
But again, probably not that point in dev for this repo...
| isSuccess: false, | ||
| error: errorMessage | ||
| error: errorMessage, | ||
| recipientPartyId: source === 'sme' ? 'PADPIDA2007040502I' : undefined |
There was a problem hiding this comment.
now that I've seen it thrice, definitely should be a helper
| // Handle case where there are common prefixes (folder structure) | ||
| if (prefixes && prefixes.length > 0) { | ||
| const batchSize = 20 | ||
| const batchSize = 100 |
There was a problem hiding this comment.
Super curious how many simultaneous connections the node server can have open to a single host and whether 100 things will actually fire off at once.
| }) | ||
| ) | ||
|
|
||
| // Process all files in chronological order |
There was a problem hiding this comment.
I will admit I was at first confused as to how this is ensuring chronological ordering haha. But I see that's in the implementation details of processS3Contents
| .sort((a, b) => { | ||
| const timestampA = a!.messageTimestamp || '' | ||
| const timestampB = b!.messageTimestamp || '' | ||
| return timestampA.localeCompare(timestampB) |
There was a problem hiding this comment.
Resisting the urge to go down a rabbit hole to determine if locale actually matters when comparing timestamps strings.
Also, feels like we maybe shouldn't do that lexicographically but should parse them to actual numbers and do the comparison with plain ole gt/lt
| for (const file of sortedFiles) { | ||
| const xmlUrl = `s3://` + join(bucket, file!.key) | ||
| console.log('parsing', xmlUrl, 'timestamp:', file!.messageTimestamp) | ||
| const releases = (await parseDdexXml(source, xmlUrl, file!.xml)) || [] |
There was a problem hiding this comment.
So just curious, the parsing of the files changes based on the order you parse them in? Or is it just the processing of them afterwards that matters? Feels like a serial process of parsing files is slower than it could be. But if this is doing more than parsing, I could see why it matters.
| const Bucket = s3url.host | ||
| const Key = s3url.pathname.substring(1) | ||
| const source = sources.findByXmlUrl(xmlUrl) | ||
| const s3 = dialS3(source) |
| const source = sources.findByXmlUrl(xmlUrl) | ||
| const s3 = dialS3(source) | ||
| const command = new GetObjectCommand({ Bucket, Key }) | ||
| const signed = await getSignedUrl(s3 as any, command as any, { |
| releases: DDEXRelease[], | ||
| isSuccess: boolean, | ||
| error?: string | ||
| error?: string, |
There was a problem hiding this comment.
is this typical TS? or some other kind of error?
3 participants
This was painful!
Tested via prod s3 dump.
using the correct source config
Manually reviewed recent dump