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

: Add EccNodeSync class #685

Closed
wants to merge 3 commits into from
Closed

Conversation

manmagic3
Copy link
Contributor

This class is for inserting/iterating Node sync data

@manmagic3 manmagic3 requested a review from a team as a code owner July 3, 2024 06:40
Copy link
Contributor

@VictorCavichioli VictorCavichioli left a comment

Choose a reason for hiding this comment

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

I think we need to expose it as Configuration, like RepairHistory, it would receive the nativeConnectionProvider bean and get the nodes to populate in the startup. I just don't know if we can define optional configurations based on the nativeConnectionProvider that we are using.

}



Copy link
Contributor

Choose a reason for hiding this comment

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

Remove empty lines

node.getHostId());
}

@VisibleForTesting
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say that you can let it as public, as we'd use this method to populate initial nodes state on cassandra

public class EccNodesSyncTest
{
@Mock
private CqlSession mockSession;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add empty line between the fields

@VictorCavichioli
Copy link
Contributor

@manmagic3 plese refer the issue id and include your changes on the docs

@VictorCavichioli
Copy link
Contributor

As Manish didn't had time to finish it, I finished as my next task depends on this, the patch will be done in a separate PR

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.

None yet

2 participants