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

fix(ibmi): reconnect when connection drops because of 08S01 error #16259

Merged
merged 9 commits into from
Sep 23, 2023

Conversation

rcronin
Copy link
Contributor

@rcronin rcronin commented Jul 12, 2023

Pull Request Checklist

  • [ X ] Have you added new tests to prevent regressions?
    Tested via add a retry block on the Sequelize config locally and verified change allows connection to be re-established.
  • If a documentation update is necessary, have you opened a PR to the documentation repository?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Does the name of your PR follow our conventions?

Description Of Change

Changes here include a way to get sequelize when the IBM i connection drops. This change checks a specific SQL state on the IBM i and utilizes the ConnectionRefusedError to trigger a reconnect.

@rcronin rcronin changed the title Fix IBM i reconnection issue fix: IBM i reconnection issue Jul 12, 2023
@ephys ephys requested review from ephys and WikiRik July 21, 2023 13:34
@sdepold
Copy link
Member

sdepold commented Jul 24, 2023

I guess some sort of test would be much appreciated :)

@rcronin
Copy link
Contributor Author

rcronin commented Jul 24, 2023

I guess some sort of test would be much appreciated :)

I'd be happy to add them but I don't see any unit tests even started for the ibm i...

@WikiRik
Copy link
Member

WikiRik commented Jul 24, 2023

Unit tests are ran by the CI, using yarn lerna run test-unit-ibmi --scope=@sequelize/core. Integration tests are indeed not run since we as maintainers do not currently have access to an instance running IBM i

@rcronin
Copy link
Contributor Author

rcronin commented Aug 7, 2023

With a change like this, is it the assumption I need to start creating the integration tests for this to get merged?

@WikiRik
Copy link
Member

WikiRik commented Aug 7, 2023

It would be nice if you could add a test so we can have an idea when this issue would occur and be fixed by this. Although we wouldn't actually be able to test it since we do not have access to an instance but we would at least have a general idea for the future

@WikiRik
Copy link
Member

WikiRik commented Sep 23, 2023

A test would have been nice, but I looked into it and it should be fine. Like I mentioned before; it's not like the test would get ran anyway

@WikiRik WikiRik changed the title fix: IBM i reconnection issue fix(ibmi): reconnect when connection drops because of 08S01 error Sep 23, 2023
@WikiRik WikiRik added this pull request to the merge queue Sep 23, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 23, 2023
@WikiRik WikiRik added this pull request to the merge queue Sep 23, 2023
Merged via the queue into sequelize:main with commit 3e993f3 Sep 23, 2023
50 of 56 checks passed
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.

None yet

3 participants