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

fix: fixes a regression that requires a log file in multi-tenant mode #2918

Merged
merged 4 commits into from
Apr 26, 2024

Conversation

amanji
Copy link
Contributor

@amanji amanji commented Apr 25, 2024

This fixes a regression introduced by #2870 that required a log file to be present on file when using the default config setup for multi-tenant mode. The log file is only required if the user specifically sets the --log-file startup param.

Signed-off-by: Akiff Manji <akiff.manji@quartech.com>
Signed-off-by: Akiff Manji <akiff.manji@quartech.com>
Signed-off-by: Akiff Manji <akiff.manji@quartech.com>
@jamshale
Copy link
Contributor

I don't see anything wrong with the changes made, but several integration tests are failing related to the logging config.

@swcurran
Copy link
Member

@amanji — are you on this?

@amanji
Copy link
Contributor Author

amanji commented Apr 26, 2024

@swcurran Yes. Just running local tests

Signed-off-by: Akiff Manji <akiff.manji@quartech.com>
Copy link

sonarcloud bot commented Apr 26, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@jamshale
Copy link
Contributor

FYI. The last fail wasn't related to the PR. I've seen it a couple times lately and I'm pretty sure it's related to the von-network nodes getting out of sync.

It may help to wait longer for this test. Or a script to occasionally sync up the nodes.

Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

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

Honestly, I'm still a bit confused by all this logging files and different configurations. I'm going to approve it though as you have detailed comments and looks like you've got a solid understanding.

@amanji
Copy link
Contributor Author

amanji commented Apr 26, 2024

It has been just as perplexing to me having gone through the code a few times. There are likely some historical design decisions that I'm not aware of. I do think this can be distilled further down but probably best left for another refactor.

@swcurran
Copy link
Member

From an outsider, I can’t figure out why there is any configuration beyond the location of the file. AFAIK, what we need is to add the tenant ID to the log, so that later when the logs are processed, they can be separated. That should be the same whether it is single vs. multi-tenent.

Or are we splitting the logs at the source and writing each one to a separate file?

Is there a description of the design that to review. I’ve not looked at it, so I could be way off.

@amanji amanji merged commit 2c3d1a4 into hyperledger:main Apr 26, 2024
8 checks passed
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.

None yet

3 participants