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

[grid] Integrating NewSessionQueuer with Distributor. #8747

Merged
merged 23 commits into from
Nov 8, 2020

Conversation

pujagani
Copy link
Contributor

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Integrated Distributor with the NewSessionQueuer.

  1. Update build files of Distributor.
  2. Add new session request listener to LocalDistributor. Reject request if it exceeds the timeout or does not have a slot for the capability. Retry request if all slots are busy.
  3. Update Distributor instance declaration in all related tests.
  4. Update Hub and Standalone commands.

Motivation and Context

A new session request is added to the queue by the NewSessionQueuer, the Distributor picks up the request and creates a session. The changes add the functionality where the Distributor has an event handler for the new session request. It also includes logic to retry if slots are busy or to reject a request immediately if no slots have the capability requested.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

shs96c
shs96c previously requested changes Sep 30, 2020
Copy link
Member

@shs96c shs96c left a comment

Choose a reason for hiding this comment

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

I see the way you're heading, and it makes sense, but there are some issues that need addressing. Notably, using exceptions for control flow is generally a code smell. Using the messages in exceptions for control flow is downright dangerous --- a casual developer has no idea of the importance of those messages, as they're normally assumed to be for humans to read.

I've also made a set of style nits --- I know we don't follow the style consistently on this project, but it'd be nice to follow the Google Style Guide for Java when we're coding, please.

@pujagani pujagani force-pushed the session-queuer-distributor branch 2 times, most recently from 960b7a8 to b695e3e Compare October 13, 2020 06:16
@diemol diemol dismissed shs96c’s stale review November 8, 2020 22:26

Changes were implemented

@diemol diemol merged commit 23bc00e into SeleniumHQ:trunk Nov 8, 2020
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