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 Port Binding Issue During Testing #382

Merged
merged 10 commits into from
Dec 6, 2023
Merged

Fix Port Binding Issue During Testing #382

merged 10 commits into from
Dec 6, 2023

Conversation

neeetman
Copy link
Contributor

@neeetman neeetman commented Oct 25, 2023

This PR addresses the issue where tests would fail if the designated port was already in use.

[INFO ] 2023-10-25 07:18:47,185 [main]  com.oltpbenchmark.benchmarks.resourcestresser.TestResourceStresserBenchmark setUp - starting HSQLDB server for test [testGetTransactionType] on port [9099]
[Server@191774d6]: [Thread[HSQLDB Server @191774d6,5,main]]: run()/openServerSocket(): 
java.net.BindException: Address already in use

Changes:

  • Modified the setUp method in TestResourceStresserBenchmark class.
  • Added logic to try different ports using ServerSocket to find an available port.
  • The system will attempt up to MAX_TRIES to find an open port before throwing an exception.
  • The the system will make a thorough effort in finding an available port.

@bpkroth bpkroth self-assigned this Oct 26, 2023
@apavlo
Copy link
Member

apavlo commented Oct 27, 2023

@neeetman What problem is this trying to solve in testing? If the port is in use, then the test environment is wrong not the code.

@neeetman
Copy link
Contributor Author

@neeetman What problem is this trying to solve in testing? If the port is in use, then the test environment is wrong not the code.

Each test requires applying for a new port. However, sometimes other services running on the server might already be occupying that port. It might be better to consider skipping the occupied ports instead of directly terminating the test with an error.

Copy link
Collaborator

@bpkroth bpkroth left a comment

Choose a reason for hiding this comment

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

LGTM. Minor tweak suggested.

@neeetman neeetman closed this Nov 19, 2023
@bpkroth
Copy link
Collaborator

bpkroth commented Dec 5, 2023

@neeetman, just noticed you closed this before merging. Any particular reason?

@bpkroth bpkroth reopened this Dec 5, 2023
@neeetman
Copy link
Contributor Author

neeetman commented Dec 6, 2023

@neeetman, just noticed you closed this before merging. Any particular reason?

Sorry, I previously noticed that 'Some checks were not successful,' so I closed it.

@bpkroth
Copy link
Collaborator

bpkroth commented Dec 6, 2023

@neeetman, just noticed you closed this before merging. Any particular reason?

Sorry, I previously noticed that 'Some checks were not successful,' so I closed it.

Ah, for future, just ping back about that. Happy to help look into those.

That one looks spurious. Seems to have failed in the nightly run a few times as well, so probably needs separate investigation. I'll file an issue on that one and follow up separately.

#405

@bpkroth bpkroth enabled auto-merge (squash) December 6, 2023 15:57
@bpkroth bpkroth merged commit 60aa59a into cmu-db:main Dec 6, 2023
105 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.

3 participants