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

FISH-898 Added timeout option to instance commands #5630

Merged
merged 5 commits into from
Apr 4, 2022

Conversation

kalinchan
Copy link
Member

@kalinchan kalinchan commented Feb 28, 2022

Description

Added --timeout option

Important Info

Blockers

N/A

Testing

New tests

N/A

Testing Performed

Built Payara and tested timeout

Testing Environment

ubuntu 20.04 maven 3.6.3 openjdk version "1.8.0_302"

Documentation

payara/Payara-Community-Documentation#296

Notes for Reviewers

@kalinchan
Copy link
Member Author

jenkins test please

@kalinchan
Copy link
Member Author

jenkins test please

Copy link
Member

@Pandrex247 Pandrex247 left a comment

Choose a reason for hiding this comment

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

I have issues with this.

I argue that this should also cover StartLocalInstanceCommand, StopLocalInstanceCommand, and RestartLocalInstanceCommand - they also manage the lifecycle of an instance.

I also argue that the restart-cluster, start-cluster, and stop-cluster commands should also have the timeout option.

I also think the way the start-instance command does it is a bit nicer (sidenote, we now have two different impls of how to perform a timeout) - utlising the internal managed executor service to wait after executing the command rather than spinning up a standalone unmanaged thread and executing the entire command within that.

    @Inject
    private PayaraExecutorService executor;
...
    /**
     * Poll for the specified amount of time to check if the instance is running.
     * Returns whether the instance was started before the timeout.
     * 
     * @return true if the instance started up, or false otherwise.
     */
    private boolean pollForLife(Server instance) {

        // Start a new thread to check when the instance has started
        CountDownLatch instanceTimeout = new CountDownLatch(1);
        ScheduledFuture<?> instancePollFuture = executor.scheduleWithFixedDelay(() -> {
            if (instance.isRunning()) {
                instanceTimeout.countDown();
            }
        }, 500, 500, MILLISECONDS);

        // If the timeout is reached, the instance isn't started so return false
        try {
            instanceTimeout.await(timeout, SECONDS);
        } catch (InterruptedException e) {
            return false;
        } finally {
            instancePollFuture.cancel(true);
        }
        
        return true;
    }

Also the restart-instance command timeout probably isn't working as expected. It seems to say the instance restarted successfully even if it hasn't actually restarted yet.
Running the below it reports that the instance was restarted successfully before the instance was even shut down.

restart-instance --timeout 1 Tender-Carp
Tender-Carp was restarted.
Command restart-instance executed successfully.

Copy link
Member

@Pandrex247 Pandrex247 left a comment

Choose a reason for hiding this comment

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

You've still got competing timeouts

@kalinchan
Copy link
Member Author

jenkins test please

@kalinchan kalinchan requested a review from Pandrex247 April 1, 2022 13:22
@kalinchan
Copy link
Member Author

jenkins test please

Copy link
Member

@Pandrex247 Pandrex247 left a comment

Choose a reason for hiding this comment

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

Missing man page updates:

  • stop-local-instance.1
  • restart-local-instance.1
  • start-local-instance.1
  • stop-instance.1
  • restart-instance.1
  • start-instance.1
  • stop-cluster.1
  • start-cluster.1

@kalinchan kalinchan requested a review from Pandrex247 April 4, 2022 10:55
@Pandrex247
Copy link
Member

Jenkins test please

@Pandrex247 Pandrex247 merged commit bbb601f into payara:master Apr 4, 2022
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.

2 participants