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

UC Volume ingestion: uniform behavior on SQL REMOVE #249

Merged
merged 1 commit into from
Apr 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions lib/DBSQLSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -280,11 +280,14 @@ export default class DBSQLSession implements IDBSQLSession {
const agent = await connectionProvider.getAgent();

const response = await fetch(presignedUrl, { method: 'DELETE', headers, agent });
// Looks that AWS and Azure have a different behavior of HTTP `DELETE` for non-existing files
// AWS assumes that - since file already doesn't exist - the goal is achieved, and returns HTTP 200
// Looks that AWS and Azure have a different behavior of HTTP `DELETE` for non-existing files.
// AWS assumes that - since file already doesn't exist - the goal is achieved, and returns HTTP 200.
// Azure, on the other hand, is somewhat stricter and check if file exists before deleting it. And if
// file doesn't exist - Azure returns HTTP 404
if (!response.ok) {
// file doesn't exist - Azure returns HTTP 404.
//
// For us, it's totally okay if file didn't exist before removing. So when we get an HTTP 404 -
// just ignore it and report success. This way we can have a uniform library behavior for all clouds
if (!response.ok && response.status !== 404) {
throw new StagingError(`HTTP error ${response.status} ${response.statusText}`);
}
}
Expand Down
14 changes: 11 additions & 3 deletions tests/e2e/staging_ingestion.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,17 +114,25 @@ describe('Staging Test', () => {
});

const stagingFileName = `/Volumes/${catalog}/${schema}/${volume}/${uuid.v4()}.csv`;
const localFile = path.join(localPath, `${uuid.v4()}.csv`);

// File should not exist before removing
try {
await session.executeStatement(`REMOVE '${stagingFileName}'`, { stagingAllowedLocalPath: [localPath] });
// In some cases, `REMOVE` may silently succeed for non-existing files (see comment in relevant
// part of `DBSQLSession` code). But if it fails - it has to be an HTTP 404 error
await session.executeStatement(`GET '${stagingFileName}' TO '${localFile}'`, {
stagingAllowedLocalPath: [localPath],
});
expect.fail('It should throw HTTP 404 error');
} catch (error) {
if (error instanceof StagingError) {
expect(error.message).to.contain('404');
} else {
throw error;
}
} finally {
fs.rmSync(localFile, { force: true });
}

// Try to remove the file - it should succeed and not throw any errors
await session.executeStatement(`REMOVE '${stagingFileName}'`, { stagingAllowedLocalPath: [localPath] });
});
});
Loading