Skip to content

Conversation

@rmoneys
Copy link
Contributor

@rmoneys rmoneys commented Jul 28, 2022

https://synccomputing.atlassian.net/browse/PROD-325

Start with spark_log_parser/__main__.py to see how the new event log module is used.

@rmoneys rmoneys requested a review from gorskysd July 29, 2022 01:50
@rmoneys rmoneys force-pushed the PROD-325-databricks-rollovers branch from f409cdf to 2710733 Compare July 29, 2022 02:18
Copy link
Contributor

@gorskysd gorskysd left a comment

Choose a reason for hiding this comment

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

Nice. The compression handling here is really clear, I like the design pattern you put in. Also the robust use of Path was nice, there are a lot of pieces I saw here that I wasn't familiar with.

I made some minor suggestions for change, but nothing critical IMO, so submitting my approval.

Copy link
Contributor

@gorskysd gorskysd left a comment

Choose a reason for hiding this comment

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

One last minute change, sorry!

Corner case to consider where there's only one rollover log and it's not the 0th -- in this case I believe the rollover ID check is bypassed. This is a common case when client's don't know to expect more than one eventlog. See example log in https://synccomputing.atlassian.net/browse/PROD-399?focusedCommentId=11208

@rmoneys
Copy link
Contributor Author

rmoneys commented Aug 2, 2022

Phew!
Rolled in some good feedback since e5f1974 including,

  1. Added support for s3 directories
  2. Handled missing first db rollover log corner case
  3. Removed pydantic model since it wasn't doing much besides getting in the way of injecting an s3 client to make testing easier
  4. Added more tests
  5. Misc refactor for more robust code 🤞

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 2, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 10 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@gorskysd gorskysd left a comment

Choose a reason for hiding this comment

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

Nice, this looks great, Scott.

I'm learning a lot about Path and url handling here that's super useful.

@rmoneys rmoneys merged commit 8e9543c into main Aug 2, 2022
@rmoneys rmoneys deleted the PROD-325-databricks-rollovers branch August 2, 2022 20:41
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.

3 participants