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.

Sign up for GitHub

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

Jump to bottom

fix: Add Index out of bound exception handling for PropertiesFile.save() #96

Merged
merged 2 commits into from
Nov 19, 2022
Merged

fix: Add Index out of bound exception handling for PropertiesFile.save() #96

merged 2 commits into from
Nov 19, 2022

Conversation

bohan-amplitude
Copy link
Contributor

@bohan-amplitude bohan-amplitude commented Nov 19, 2022 •
edited
Loading

Summary

Expection catch and log for PropertiesFile.save()
Change more general Exception catch for PropertiesFile.load()
Unit test for added function

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?: no

@bohan-amplitude bohan-amplitude changed the title Add Index out of bound exception handling for PropertiesFile.save() fix: Add Index out of bound exception handling for PropertiesFile.save() Nov 19, 2022
justin-fiedler
justin-fiedler approved these changes Nov 19, 2022
View reviewed changes
Copy link
Contributor

@justin-fiedler justin-fiedler left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This should workaround the issue. Were you able to locate the root cause? Is this from lack of file access, or Properties have become corrupted?

@bohan-amplitude
Copy link
Contributor Author

This should workaround the issue. Were you able to locate the root cause? Is this from lack of file access or is it the Properties have become corrupted?

I'll ask if customer can give us some log info that Marvin previous added. It sounds like a random access issue that caused by multithreading

@bohan-amplitude bohan-amplitude merged commit c6578bf into amplitude:main Nov 19, 2022
@ondrej
Copy link

ondrej commented Nov 19, 2022

Will the logs you and Marvin added provide more info about the root cause? It looks like we're logging the file path and the stack trace, but the problem appears to be with the values in the properties map.

Do we blanket catch Exception normally in our SDKs? It's normally an anti-pattern since it can potentially hide serious problems that should actually escalate instead of leaving the app in an uncertain state.

github-actions bot pushed a commit that referenced this pull request Nov 19, 2022
@amplitude-sdk-bot
chore(release): 1.5.2 [skip ci]
656586a 
## [1.5.2](v1.5.1...v1.5.2) (2022-11-19)

### Bug Fixes

* Add Index out of bound exception handling for PropertiesFile.save() ([#96](#96)) ([c6578bf](c6578bf))
@github-actions
Copy link

🎉 This PR is included in version 1.5.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuhao900914 yuhao900914 yuhao900914 approved these changes

@justin-fiedler justin-fiedler justin-fiedler approved these changes

@haoliu-amp haoliu-amp Awaiting requested review from haoliu-amp

@qingzhuozhen qingzhuozhen Awaiting requested review from qingzhuozhen

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bohan-amplitude @ondrej @justin-fiedler @yuhao900914