Skip to content

[java] Ensure purging dead nodes service interval is configurable #15175

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

Merged
merged 3 commits into from
Jan 30, 2025

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Jan 28, 2025

User description

Fixes #15168

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.

Motivation and Context

Related to #15168

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.

PR Type

Enhancement, Tests


Description

  • Added configurable interval for purging dead nodes.

  • Updated LocalDistributor to handle purge interval logic.

  • Enhanced tests to validate the new purge interval functionality.

  • Introduced default value and configuration flag for purge interval.


Changes walkthrough 📝

Relevant files
Enhancement
5 files
Hub.java
Added purge nodes interval to Hub distributor initialization.
+2/-1     
Standalone.java
Added purge nodes interval to Standalone distributor initialization.
+2/-1     
DistributorFlags.java
Added CLI flag for purge nodes interval configuration.     
+9/-0     
DistributorOptions.java
Added default and configurable purge nodes interval logic.
+12/-0   
LocalDistributor.java
Integrated purge nodes interval into LocalDistributor logic.
+14/-4   
Tests
13 files
AddingNodesTest.java
Updated tests to include purge nodes interval in distributor setup.
+10/-5   
DistributorDrainingTest.java
Enhanced draining tests to include purge nodes interval. 
+8/-4     
DistributorNodeAvailabilityTest.java
Updated node availability tests with purge nodes interval.
+12/-6   
DistributorTest.java
Enhanced distributor tests to validate purge nodes interval.
+14/-7   
SessionSchedulingTest.java
Updated session scheduling tests with purge nodes interval.
+8/-4     
LocalDistributorTest.java
Added tests for LocalDistributor with purge nodes interval.
+14/-7   
GraphqlHandlerTest.java
Updated GraphQL handler tests to include purge nodes interval.
+12/-6   
JmxTest.java
Enhanced JMX tests to validate purge nodes interval.         
+2/-1     
NewSessionCreationTest.java
Updated session creation tests with purge nodes interval.
+6/-3     
RouterTest.java
Enhanced router tests to include purge nodes interval.     
+2/-1     
SessionCleanUpTest.java
Updated session cleanup tests with purge nodes interval. 
+4/-2     
SessionQueueGridTest.java
Enhanced session queue grid tests with purge nodes interval.
+2/-1     
SessionQueueGridWithTimeoutTest.java
Updated session queue grid timeout tests with purge nodes interval.
+2/-1     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Sorry, something went wrong.

    @pujagani pujagani marked this pull request as ready for review January 28, 2025 11:10
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Input Validation

    The getPurgeNodesInterval() method allows setting interval to 0 to disable purging, but this behavior is not documented in the configuration flag description.

    public Duration getPurgeNodesInterval() {
      // If the user sets 0s or less, we default to 0s and disable the purge dead nodes service.
      int seconds =
          Math.max(
              config
                  .getInt(DISTRIBUTOR_SECTION, "purge-nodes-interval")
                  .orElse(DEFAULT_PURGE_NODES_INTERVAL),
              0);
      return Duration.ofSeconds(seconds);
    }

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Validate negative purge interval values

    Add validation to ensure purge nodes interval is not negative, as negative values
    could cause unexpected behavior. The current max() check allows negative input
    values to pass through.

    java/src/org/openqa/selenium/grid/distributor/config/DistributorOptions.java [101-110]

     public Duration getPurgeNodesInterval() {
    -  // If the user sets 0s or less, we default to 0s and disable the purge dead nodes service.
    -  int seconds =
    -      Math.max(
    -          config
    -              .getInt(DISTRIBUTOR_SECTION, "purge-nodes-interval")
    -              .orElse(DEFAULT_PURGE_NODES_INTERVAL),
    -          0);
    +  int seconds = config
    +      .getInt(DISTRIBUTOR_SECTION, "purge-nodes-interval")
    +      .orElse(DEFAULT_PURGE_NODES_INTERVAL);
    +  if (seconds < 0) {
    +    seconds = 0; // Disable purge service for negative values
    +  }
       return Duration.ofSeconds(seconds);
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves input validation by explicitly handling negative values, making the code's behavior more explicit and maintainable compared to the implicit Math.max() approach. This helps prevent potential issues with invalid configuration values.

    7
    Learned
    best practice
    Add null validation check for required config parameter to prevent potential NullPointerException

    The getPurgeNodesInterval() method should validate that the config parameter is not
    null before accessing it, to prevent potential NullPointerException. Add a null
    check using Require.nonNull() at the start of the method.

    java/src/org/openqa/selenium/grid/distributor/config/DistributorOptions.java [101-110]

     public Duration getPurgeNodesInterval() {
    +  Require.nonNull("Config", config);
       // If the user sets 0s or less, we default to 0s and disable the purge dead nodes service.
       int seconds =
           Math.max(
               config
                   .getInt(DISTRIBUTOR_SECTION, "purge-nodes-interval")
                   .orElse(DEFAULT_PURGE_NODES_INTERVAL),
               0);
       return Duration.ofSeconds(seconds);
     }
    • Apply this suggestion
    6

    Sorry, something went wrong.

    @pujagani pujagani merged commit 764a4b6 into SeleniumHQ:trunk Jan 30, 2025
    31 of 33 checks passed
    RenderMichael added a commit that referenced this pull request Jan 30, 2025
    * [java] Ensure purging dead nodes service interval is configurable (#15175)
    
    Fixes #15168
    
    * [bazel] Bump JS rulesets (#15187)
    
    * Add explicit reference to cdp types
    
    ---------
    
    Co-authored-by: Puja Jagani <puja.jagani93@gmail.com>
    Co-authored-by: Simon Stewart <simon.m.stewart@gmail.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [🚀 Feature]: Provide an Option to Disable purgeDeadNodesService in Selenium 4 Standalone Mode
    1 participant