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

feat(idpool): idpool feature for generating id's #400

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

venkyvsp
Copy link
Contributor

idpool feature for generating id's for mod pointer, routing table id , trie pointer.

@venkyvsp venkyvsp marked this pull request as ready for review September 23, 2024 07:09
@venkyvsp venkyvsp requested a review from a team as a code owner September 23, 2024 07:09
pkg/LinuxGeneralModule/lgm.go Outdated Show resolved Hide resolved
pkg/LinuxGeneralModule/lgm.go Outdated Show resolved Hide resolved
pkg/LinuxGeneralModule/lgm.go Outdated Show resolved Hide resolved
@@ -465,8 +467,12 @@ func setUpVrf(vrf *infradb.Vrf) (string, bool) {
vrf.Metadata.RoutingTable = make([]*uint32, 1)
vrf.Metadata.RoutingTable[0] = new(uint32)
var routingTable uint32
Name := vrf.Name
Copy link
Contributor

Choose a reason for hiding this comment

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

please align with Go style guide:
https://google.github.io/styleguide/go/decisions.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you overshot it :)
why _unusedids? It is almost unreadable. Use lower case for not exported members. Use camelCase naming. Do not use _ in names.

pkg/LinuxGeneralModule/lgm.go Outdated Show resolved Hide resolved
pkg/utils/Idpool.go Outdated Show resolved Hide resolved
pkg/utils/Idpool.go Outdated Show resolved Hide resolved
pkg/utils/Idpool.go Outdated Show resolved Hide resolved
pkg/utils/Idpool.go Outdated Show resolved Hide resolved
_unusedIDs []uint32 // Yet unused IDs in pool Available ids
_idsInUse map[interface{}]uint32 // Mapping key: id for currently assigned ids
_idsForReuse map[interface{}]uint32 // Mapping key: id for previously assigned ids
_refs map[uint32][]interface{}
Copy link
Contributor

Choose a reason for hiding this comment

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

refs sounds like a counter tracking how many times we requested the same id, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, its reference to the same id

Copy link
Contributor

Choose a reason for hiding this comment

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

then why do we need a slice of slices then? Can we have a slice of counters? map[uint32]uint32?

// Copyright (c) 2022-2023 Intel Corporation, or its subsidiaries.
// Copyright (C) 2023 Nordix Foundation.

// Package linuxgeneralmodule is the main package of the application
Copy link
Contributor

Choose a reason for hiding this comment

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

?

if len(ip._unusedIDs) != 0 {
// Pick an unused id
id = ip._unusedIDs[0]
ip._unusedIDs = append(ip._unusedIDs[:0], ip._unusedIDs[1:]...)
Copy link
Contributor

Choose a reason for hiding this comment

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

you are still picking the first element from the slice, which is suboptimal? If you reverse your unused id in init and will pick last element (which is more effective)?

@venkyvsp venkyvsp force-pushed the opi_idpool_main_pr branch 2 times, most recently from 322f68f to 9bfa71a Compare October 24, 2024 19:36
Signed-off-by: Venkatesh, Vemula <venkatesh.vemula@intel.com>
Signed-off-by: Atul Patel <Atul.Patel@intel.com>
Signed-off-by: Vemula Venkatesh <venkatesh.vemula@intel.com>
Signed-off-by: Vemula Venkatesh <venkatesh.vemula@intel.com>
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