Skip to content

Conversation

@Mmuzaf
Copy link
Contributor

@Mmuzaf Mmuzaf commented Feb 27, 2024

Thanks for sending a pull request! Here are some tips if you're new here:

  • Ensure you have added or run the appropriate tests for your PR.
  • Be sure to keep the PR description updated to reflect all changes.
  • Write your PR title to summarize what this PR proposes.
  • If possible, provide a concise example to reproduce the issue for a faster review.
  • Read our contributor guidelines
  • If you're making a documentation change, see our guide to documentation contribution

Commit messages should follow the following format:

<One sentence description, usually Jira title or CHANGES.txt summary>

<Optional lengthier description (context on patch)>

patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####

Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>

The Cassandra Jira

Copy link
Contributor

Choose a reason for hiding this comment

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

is this ... ok? why cant we put this to e.g. init()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The metrics should only be registered once, when the class is inited. However, since it relies on the local counters, e.g. bootstrapFilesTotal, it is easier to register them as static variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about changing init() to accept listeners... (might be on varargs)

in StorageService we do

    bootstrapper.init();
    bootstrapper.addProgressListener(progressSupport);

could not we do all of this in init() ?

Copy link
Contributor

Choose a reason for hiding this comment

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

btw is it safe if somebody, by accident, calls this method twice? what would happen then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can have only one bootstrap process running, there is a check for that:

        BootStrapper bootstrapper = new BootStrapper(getBroadcastAddressAndPort(), metadata, movements, strictMovements);
        boolean res = ongoingBootstrap.compareAndSet(null, bootstrapper);
        if (!res)
            throw new IllegalStateException("Bootstrap can be started exactly once, but seems to have already started: " + bootstrapper);

Anyway, I've moved the init to the constructor, so it should be clearer now.

@Mmuzaf Mmuzaf force-pushed the cassandra-19447 branch from 31f98c0 to d71d0ec Compare March 6, 2024 13:58
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