Skip to content

Fix: Use Go Generics to Consolidate Subset functions#5444

Open
FirePheonix wants to merge 1 commit intohyperledger:mainfrom
FirePheonix:fix/configtx-subset-generics
Open

Fix: Use Go Generics to Consolidate Subset functions#5444
FirePheonix wants to merge 1 commit intohyperledger:mainfrom
FirePheonix:fix/configtx-subset-generics

Conversation

@FirePheonix
Copy link
Copy Markdown
Contributor

  • Improvement: use of generics consolidate the earlier subset functions

Description

subsetOfGroups, subsetOfPolicies, and subsetOfValues were three identical functions differing only in map value type.

A TODO had already existed since the code was written acknowledging the duplication, but noted the signatures "need to be different".

At the time, Go had no generics, so there was no clean way to unify them without losing type safety.

Go 1.18 introduced generics. This PR resolves the TODO by replacing all three functions with a single subsetOf[V any] function, letting the compiler infer the concrete type at each call site. ZERO behavior change - the logic is identical.
Now, 60 lines -> 10 lines of code

I have also added a new test case for partial error corresponding to the changes I've done.

@FirePheonix FirePheonix requested a review from a team as a code owner April 5, 2026 07:52
@FirePheonix FirePheonix force-pushed the fix/configtx-subset-generics branch from b5f52f5 to 36631fa Compare April 5, 2026 08:00
@FirePheonix FirePheonix changed the title Fix: Fix: Use Go Generics to Consolidate Subset functions Apr 5, 2026
@pfi79
Copy link
Copy Markdown
Contributor

pfi79 commented Apr 5, 2026

It seems you've hit some unit tests.

@FirePheonix
Copy link
Copy Markdown
Contributor Author

hey @pfi79 ,
I have a feeling that the test failing is completely unrelated to my changes..
and that test is failing right now in this PR since there's a shared variable between somewhere..

it's the TestConcurrentCreateLedgerFromSnapshot

the tests are running for so long and then failing, it might even be a race condition.
maybe i;m wrong, lemme see..

@pfi79
Copy link
Copy Markdown
Contributor

pfi79 commented Apr 5, 2026

hey @pfi79 , I have a feeling that the test failing is completely unrelated to my changes.. and that test is failing right now in this PR since there's a shared variable between somewhere..

it's the TestConcurrentCreateLedgerFromSnapshot

the tests are running for so long and then failing, it might even be a race condition. maybe i;m wrong, lemme see..

  1. You can run the tests in your own brunch.
  2. You can also run them for main.

When your pipe turns green locally, I'm ready to consider it further. So far, I've restarted your pr pipe 7-8 times and so far everything is bad.

@FirePheonix
Copy link
Copy Markdown
Contributor Author

hey @pfi79 , i ran the tests on my PR replica, on shubhammms#3.

I was earlier re running all jobs by commiting on it, that didn't work.
But then I started running individual jobs, and all succeeded on that PR.

Been trying to figure it out for a whole day..
I think it's a CI issue which is failing due to load on the CI workers, you can observe that in the attempts, they fail on different tests in different different attempts.

@FirePheonix
Copy link
Copy Markdown
Contributor Author

image

It ran successfully on attempt 10.
then failed in the next 2 again.

@FirePheonix
Copy link
Copy Markdown
Contributor Author

YES.
the current attempt passed all.

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