sonic-net / sonic-sairedis

SAI object interface to Redis database, as used in the SONiC project
Other
56 stars 265 forks source link

[Link event damping] Add concurrent queue. #1297

Closed Ashish1805 closed 10 months ago

Ashish1805 commented 11 months ago

HLD: https://github.com/sonic-net/SONiC/pull/1071

Ashish1805 commented 11 months ago

cc: @bhagatgit @eddyk-nvidia @mikeberesford

Ashish1805 commented 11 months ago

Unmanageable to review, too many changes, too many files affected in 1 pr, low code quality, not following existing file standards, many new classes, each new class can be a separate code review, new port custom range attributes seems better to be added in SAI repo instead of here, causing to implement custom Metadata where SAI generates that automatically

Thanks @kcudnik for taking a look.

On the issue of port custom attribute, this is added in sairedis.h in sonic-sairedis as per agreed upon in HLD https://github.com/sonic-net/SONiC/blob/master/doc/link_event_damping/Link-event-damping-HLD.md#sairedis-api

In the existing sonic-sairedis code, I see there are mix of different standards followed. I couldn't find a guideline or doc for sonic standard code, can you please share them Or if you suggest any existing code file that can be a good reference for sonic standard?

Thank you.

kcudnik commented 11 months ago

Unmanageable to review, too many changes, too many files affected in 1 pr, low code quality, not following existing file standards, many new classes, each new class can be a separate code review, new port custom range attributes seems better to be added in SAI repo instead of here, causing to implement custom Metadata where SAI generates that automatically

Thanks @kcudnik for taking a look.

On the issue of port custom attribute, this is added in sairedis.h in sonic-sairedis as per agreed upon in HLD https://github.com/sonic-net/SONiC/blob/master/doc/link_event_damping/Link-event-damping-HLD.md#sairedis-api

In the existing sonic-sairedis code, I see there are mix of different standards followed. I couldn't find a guideline or doc for sonic standard code, can you please share them Or if you suggest any existing code file that can be a good reference for sonic standard?

Thank you.

I keep the standard in this entire repo, there is no specific documents but 90% of files are kept in very good shape, please take a look on more files and point me to the one you were looking to, I pointed you more obvious issues

mikeberesford commented 11 months ago

Unmanageable to review, too many changes, too many files affected in 1 pr, low code quality, not following existing file standards, many new classes, each new class can be a separate code review, new port custom range attributes seems better to be added in SAI repo instead of here, causing to implement custom Metadata where SAI generates that automatically

Thanks @kcudnik for taking a look. On the issue of port custom attribute, this is added in sairedis.h in sonic-sairedis as per agreed upon in HLD https://github.com/sonic-net/SONiC/blob/master/doc/link_event_damping/Link-event-damping-HLD.md#sairedis-api In the existing sonic-sairedis code, I see there are mix of different standards followed. I couldn't find a guideline or doc for sonic standard code, can you please share them Or if you suggest any existing code file that can be a good reference for sonic standard? Thank you.

I keep the standard in this entire repo, there is no specific documents but 90% of files are kept in very good shape, please take a look on more files and point me to the one you were looking to, I pointed you more obvious issues

I'd (strongly) suggest that SONiC adopt a published coding standard, and perhaps we can help with that. As an example (admitting bias here!) Google publishes https://google.github.io/styleguide/ - I'm not suggesting that SONiC adopt this guide, just listing it as an example. Having this will both prevent back-and-forth and will help reduce load on reviewers.

kcudnik commented 11 months ago

Yes, we will adopt or create some standard at some point but not at such big reviews, usually anyone is adding couple linea of code and not many classes and changing 45 files, one by one is able to adopt code

Ashish1805 commented 11 months ago

@kcudnik I was wondering which tool do you use or will recommend to use to format the syncd code during development? Manually formatting the files one by one is time consuming and prone to missing the formatting.

I am mainly looking for formatter that helps to format: 1) { starts in new line, 2) space formatting for each line, 3) width for each line for code and comments, etc. Thanks.

kcudnik commented 11 months ago

@kcudnik I was wondering which tool do you use or will recommend to use to format the syncd code during development? Manually formatting the files one by one is time consuming and prone to missing the formatting.

I am mainly looking for formatter that helps to format: 1) { starts in new line, 2) space formatting for each line, 3) width for each line for code and comments, etc. Thanks.

i dont use any specific tool, i put { in new line manually, and i use formating spacing in vim select code and press "="

kcudnik commented 10 months ago

HLD: https://github.com/sonic-net/SONiC/pull/1071