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

Error handling #71

Merged
merged 9 commits into from
Sep 26, 2023
Merged

Error handling #71

merged 9 commits into from
Sep 26, 2023

Conversation

Ansh5461
Copy link
Contributor

No description provided.

async for chunk in self.read_chunks(file):
yield CollectedBytes(file=file_path, data=chunk, error=None)
except PermissionError as exc:
print(f"Unable to open this file {file_path}, getting error as {exc}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should raise exceptions we fail file read and or permission most of the time it will be issue with user

try:
self.client = storage.Client.from_service_account_info(self.credentials)
except FileNotFoundError as exc:
raise common_errors.FileNotFoundError(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be connection error or ( failed to initial GCS bucket client)

# Handle exceptions gracefully, e.g., log the error
print(f"An error occurred while streaming blob: {e}")
except PermissionError as exc:
print(f"Unable to open this file {blob_file}, getting error as {exc}")
Copy link
Contributor

@saraswatpuneet saraswatpuneet Sep 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same if we can't get to file there is issue with container and we should stop that specific collector instead of doing a partial pull with missing files

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So raise here

except PermissionError as exc:
print(f"Unable to open this file {blob_file}, getting error as {exc}")
except OSError as exc:
print(f"Getting OS Error on file {blob_file}, as {exc}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also handle generic Exception as last option which will catch anything we cant predict so there will be an extra catch for just Exception

processed_text = await self.process_data(text)
yield processed_text
except Exception as exc:
yield ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's yield None here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of empty, and possibly at other places, that way at the queue side we will know that file ended and new started

Copy link
Contributor

@saraswatpuneet saraswatpuneet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments

Copy link
Contributor

@saraswatpuneet saraswatpuneet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job! Will do indepth review but looks great

Copy link
Contributor

@saraswatpuneet saraswatpuneet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good we can cover future enhancement in later stories

Good job

@Ansh5461 Ansh5461 merged commit 7dd9feb into main Sep 26, 2023
@Ansh5461 Ansh5461 deleted the error-handling branch September 26, 2023 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants