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

IGNITE-21911 Change API usage of Placement driver in Index module from TablePartitionId to ZonePartitionId #100

Merged
merged 11 commits into from
Apr 29, 2024

Conversation

alievmirza
Copy link

@alievmirza alievmirza commented Apr 18, 2024

@alievmirza alievmirza force-pushed the ignite-21911-in-feature-branch branch from e10f113 to 4d10b39 Compare April 18, 2024 22:01
@alievmirza alievmirza changed the title Ignite 21911 in feature branch IGNITE-21911 Change API usage of Placement driver in Index module from TablePartitionId to ZonePartitionId Apr 19, 2024
.enlistmentConsistencyToken(meta.getStartTime().longValue())
.groupId(partId)
// TODO: discuss this timeout
.timeout(10)

Choose a reason for hiding this comment

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

Extract to class scope constant?

@@ -339,11 +340,13 @@ private CompletableFuture<Boolean> primaryReplicaEventListener(
Consumer<TablePartitionId> action
) {
return inBusyLock(busyLock, () -> {
if (!(eventParameters.groupId() instanceof TablePartitionId)) {
if (!(eventParameters.groupId() instanceof ZonePartitionId)) {

Choose a reason for hiding this comment

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

Why is it not an assertion here?

Copy link
Author

Choose a reason for hiding this comment

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

I prefer to leave it as it was in the main

@@ -292,11 +293,13 @@ void stop() {

private void onPrimaryReplicaChanged(PrimaryReplicaEventParameters primaryReplicaEvent) {
inBusyLock(busyLock, () -> {
if (!(primaryReplicaEvent.groupId() instanceof TablePartitionId)) {
if (!(primaryReplicaEvent.groupId() instanceof ZonePartitionId)) {

Choose a reason for hiding this comment

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

This condition can be replaced with assert.

Copy link
Author

Choose a reason for hiding this comment

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

I prefer to leave it as it was in the main

* @return Future that contains a result.
*/
private CompletableFuture<Void> processWaitReplicaStateMessage(WaitReplicaStateMessage msg) {
LOG.info("Received WaitReplicaStateMessage for replica belonging to group=" + groupId());

Choose a reason for hiding this comment

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

Use our log format.
("WaitReplicaStateMessage was received [group={}]", groupId())

Copy link
Author

Choose a reason for hiding this comment

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

fixed

.enlistmentConsistencyToken(meta.getStartTime().longValue())
.groupId(partId)
// TODO: discuss this timeout

Choose a reason for hiding this comment

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

TODO without an issue number.

* It delegates calls to the original {@link PlacementDriver} and after that sends {@link WaitReplicaStateMessage}
* which calls {@link org.apache.ignite.internal.replicator.Replica#waitForActualState(long)}.
*/
public class ReplicaAwareLeaseTracker extends AbstractEventProducer<PrimaryReplicaEvent, PrimaryReplicaEventParameters> implements

Choose a reason for hiding this comment

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

I would inject the implementation into the lease tracker.

@@ -239,6 +239,7 @@ public static int getTableIdStrict(CatalogService catalogService, String tableNa
return getTableStrict(catalogService, tableName, timestamp).id();
}


Copy link

Choose a reason for hiding this comment

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

Horrible, disgusting mistake. Please remove this empty line.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@alievmirza alievmirza changed the base branch from ignite-18991-WIP to ignite-collocation-feature April 29, 2024 15:07
@alievmirza alievmirza merged commit aaee70b into ignite-collocation-feature Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants