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

network service improvements #847

Closed
wants to merge 8 commits into from
Closed

network service improvements #847

wants to merge 8 commits into from

Conversation

adecaro
Copy link
Contributor

@adecaro adecaro commented Feb 4, 2025

This PR does the following:

  • It avoid unnecessary duplicates of the network instances generated by the network service
  • It rework the fabric finality and lookup listeners

Signed-off-by: Angelo De Caro <[email protected]>
Signed-off-by: Angelo De Caro <[email protected]>
Signed-off-by: Angelo De Caro <[email protected]>
Signed-off-by: Angelo De Caro <[email protected]>
@adecaro adecaro self-assigned this Feb 5, 2025
@adecaro adecaro changed the title check network id network service improvements Feb 5, 2025
Signed-off-by: Angelo De Caro <[email protected]>
Signed-off-by: Angelo De Caro <[email protected]>
Copy link
Contributor

@alexandrosfilios alexandrosfilios left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -60,8 +60,8 @@ var (

AllTestTypes = []*InfrastructureType{
WebSocketNoReplication,
LibP2PNoReplication,
WebSocketWithReplication,
// LibP2PNoReplication,
Copy link
Contributor

Choose a reason for hiding this comment

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

not to forget

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

defer close(ch)

// group keys by namespace
keysByNS := map[driver2.Namespace][]string{}
Copy link
Contributor

Choose a reason for hiding this comment

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

use PKey to have some context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

// group keys by namespace
keysByNS := map[driver2.Namespace][]string{}
for k, v := range evicted {
ns := v[0].Namespace()
Copy link
Contributor

Choose a reason for hiding this comment

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

ns := slices.GetAny(v).Namespace()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

// we are abusing TxID to carry the name of the keys we are looking for.
// Keys are supposed to be unique
keys := collections.Keys(evicted) // These are the state keys we are looking for
results := collections.NewSet(keys...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we create the results here and not in queryByID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed this

Channel *fabric.Channel
}

func (q *DeliveryScanQueryByID) QueryByID(ctx context.Context, lastBlock driver2.BlockNum, evicted map[driver2.TxID][]finality.ListenerEntry[TxInfo]) (<-chan []TxInfo, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can call this startBlock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

return ch, nil
}

func (q *DeliveryScanQueryByID) queryByID(ctx context.Context, results collections.Set[string], ch chan []TxInfo, lastBlock uint64, evicted map[driver2.TxID][]finality.ListenerEntry[TxInfo]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can call it chaincodeScan or chaincodeDeliveryScan instead of deliveryScan. to distinguish it from th other implementation that only uses the delivery service

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

return ch, nil
}

func (q *DeliveryScanQueryByID) queryByID(ctx context.Context, results collections.Set[string], ch chan []TxInfo, lastBlock uint64, evicted map[driver2.TxID][]finality.ListenerEntry[TxInfo]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to use the generic TxInfo? This service is only applicable for the specific txInfo struct and you don't need to expose Namespace() for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it doesn't matter.

return ch, nil
}

func (q *DeliveryScanQueryByID) queryByID(ctx context.Context, results collections.Set[string], ch chan []TxInfo, lastBlock uint64, evicted map[driver2.TxID][]finality.ListenerEntry[TxInfo]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

results is a confusing name. lets call it remainingKeys or something similar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

logger.Errorf("failed unmarshalling results for query by ids [%v]: [%s]", keys, err)
return
}
infos := make([]TxInfo, 0, len(values))
Copy link
Contributor

Choose a reason for hiding this comment

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

we can call them found and notFound to make it a bit more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

}

if startDelivery {
startingBlock := finality.MaxUint64(1, lastBlock-10)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make a constant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

Signed-off-by: Angelo De Caro <[email protected]>
@mergify mergify bot mentioned this pull request Feb 5, 2025
@adecaro adecaro closed this Feb 5, 2025
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.

2 participants