-
Notifications
You must be signed in to change notification settings - Fork 44
Implement onNeighborListUpdated event #3076
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
Implement onNeighborListUpdated event #3076
Conversation
… neighbors that respond too slow
packages/trackerless-network/src/logic/neighbor-discovery/NeighborUpdateManager.ts
Outdated
Show resolved
Hide resolved
packages/trackerless-network/src/logic/neighbor-discovery/NeighborUpdateManager.ts
Outdated
Show resolved
Hide resolved
Should fix the merge conflicts with the versioning in package.json files |
…tachannels-to-the-users-of-trackerless-network
packages/trackerless-network/src/content-delivery-layer/NodeList.ts
Outdated
Show resolved
Hide resolved
@@ -17,6 +16,7 @@ export interface DiscoveryLayerNode { | |||
off<T extends keyof DiscoveryLayerNodeEvents>(eventName: T, listener: () => void): void | |||
once<T extends keyof DiscoveryLayerNodeEvents>(eventName: T, listener: (peerDescriptor: PeerDescriptor) => void): void | |||
once<T extends keyof DiscoveryLayerNodeEvents>(eventName: T, listener: () => void): void | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove unnecessary added line
@@ -53,10 +53,12 @@ export class NeighborUpdateManager { | |||
const res = await this.createRemote(neighbor.getPeerDescriptor()).updateNeighbors(this.options.streamPartId, neighborDescriptors) | |||
const nodeId = toNodeId(neighbor.getPeerDescriptor()) | |||
this.options.neighbors.get(nodeId)!.setRtt(Date.now() - startTime) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unnecessary adde lines in this file
this.currentBufferedAmount = bufferedAmountRemainder | ||
// Update bufferedAmount in statistics but keep the upload rate the same | ||
this.statistics.bufferedAmount = this.currentBufferedAmount | ||
this.emit('statisticsUpdated', this.statistics) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this event end up being called quite often? Could there be some performance related if the bufferedAmount changes on each send
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buffered amount should stay at 0 in normal operation, and be non-zero only if you are sending too fast. Maybe we should test this out with streamrtv by making a rc release of this branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I suppose it would make sense to do an internal release of these changes
…tachannels-to-the-users-of-trackerless-network
Limitations: