[Link event damping] Add concurrent queue. #1297

Ashish1805 · 2023-09-28T09:36:48Z

kcudnik

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

kcudnik · 2023-09-28T10:21:30Z

syncd/ConcurrentQueue.h

+  SWSS_LOG_ENTER();
+
+  std::lock_guard<std::mutex> mutex_lock(mutex_);
+  if (queue_.empty()) {


All { should start in new line

kcudnik

Give up on this review address comments

kcudnik · 2023-09-28T10:21:52Z

syncd/ConcurrentQueue.h

+  // size.
+  static constexpr size_t kUnlimited = 0;
+
+  std::mutex mutex_;


All members should be prefixed m_

kcudnik · 2023-09-28T10:22:12Z

syncd/ConcurrentQueue.h

+  SWSS_LOG_ENTER();
+
+  std::lock_guard<std::mutex> mutex_lock(mutex_);
+  return queue_.empty();


Empty line before review

kcudnik · 2023-09-28T10:22:42Z

syncd/LinkEventDamper.cpp

+
+  sai_object_id_t vid;
+  bool rid_to_vid_mapping_found =
+      vid_translator_->tryTranslateRidToVid(data.port_id, vid);


Big lump of code totally unreadable

Ashish1805 · 2023-09-28T17:00:15Z

cc: @bhagatgit @eddyk-nvidia @mikeberesford

Ashish1805 · 2023-09-28T17:12:15Z

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 · 2023-09-29T07:17:56Z

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 · 2023-09-29T15:47:47Z

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 · 2023-09-29T17:07:01Z

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

kcudnik · 2023-09-30T08:28:50Z

syncd/ConcurrentQueue.h

+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0


@lguohan do we allow each file to have different license?

Are you saying none of the files that we are trying to upstream can have license?

cc: @mikeberesford @bhagatgit

I think we have a license for entire repo not per single file

@lguohan @kcudnik Can you provide more details on the licensing.
On repos that have multiple contributors, does this mean that the contributor that started the repo would add license and others would not ?

Thanks. Addressed the review comments. Build issue is because of spell checker (aspellcheck.pl) failing due to it complaining about LLC in "Google LLC" in the copyright header. Not sure why LLC is causing spell check issue though.

@kcudnik We are still getting the licensing header issue resolved internally. Given that it is taking time, I have started generating the remaining commits to this PR. Can you please review them too? I dont know if I was supposed to generated separate PR for each commit or all the commits can be generated in same PR. Given these commits are dependent, generating multiple commits in same PRs sounds reasonable. Thanks.

Think you will hav3 still multiple classes and many many code in one pr, I would advise to have separate pr for specific modules

Just to confirm, what do you mean exactly by separate PR here? Does it mean each commit should have its own PR, for example: commit 1 in PR https://github.com/sonic-net/sonic-sairedis/pull/XXXX, commit 2 in https://github.com/sonic-net/sonic-sairedis/pull/YYYY, commit 3 in https://github.com/sonic-net/sonic-sairedis/pull/ZZZZ, and so on?

Or you are saying multiple commits in this same PR is ok but each commit should not have multiple classes?

A lot of code depend on each other. For example: commit 2 in this PR depends on commit 1 in the PR. So, if we generate separate separate PR for each commits, they will not compile since dependency will not be present.

no, not the commit, but if you are planning to add many big classes, it make sense to add each class separately, since we squash later all the commits to 1 commit on master, and if you add 40 files in one commit that will be overkill and unmanagable

syncd/ConcurrentQueue.h

Ashish1805 · 2023-10-11T21:04:25Z

@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 · 2023-10-12T09:04:33Z

@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 "="

- Class to handle the port state change event callback from SAI and sending the events to syncd main thread for processing by link event damper. Depends on: sonic-net#1297 HLD: sonic-net/SONiC#1071

…ing. - This queue will be used to enqueue the port state change events by NotificationHandler and dequeued by syncd main thread and processed in link event damping logic. HLD: sonic-net/SONiC#1071

- Class to handle the port state change event callback from SAI and sending the events to syncd main thread for processing by link event damper. Depends on: sonic-net#1297 HLD: sonic-net/SONiC#1071

syncd/ConcurrentQueue.h

kcudnik · 2023-10-24T16:17:37Z

HLD: sonic-net/SONiC#1071

- Class to handle the port state change event callback from SAI and sending the events to syncd main thread for processing by link event damper. Depends on: sonic-net#1297 HLD: sonic-net/SONiC#1071

- Class to handle the port state change event callback from SAI and sending the events to syncd main thread for processing by link event damper. Depends on: #1297 HLD: sonic-net/SONiC#1071

What I did Added support for link event damping in SWSS. Required Syncd PR: sonic-net/sonic-sairedis#1297 CLI PR: sonic-net/sonic-utilities#3001 HLD: https://github.com/sonic-net/SONiC/blob/master/doc/link_event_damping/Link-event-damping-HLD.md Why I did it How I verified it Use the config interface damping CLI to set the port attributes on the switch and observe that Syncd processes link event damping parameters.

Ashish1805 marked this pull request as draft September 28, 2023 09:37

kcudnik requested changes Sep 28, 2023

View reviewed changes

kcudnik reviewed Sep 28, 2023

View reviewed changes

kcudnik requested changes Sep 28, 2023

View reviewed changes

Ashish1805 mentioned this pull request Sep 28, 2023

Link event damping HLD sonic-net/SONiC#1071

Merged

Ashish1805 force-pushed the ledamping branch from 46b3ef2 to 975ab68 Compare September 29, 2023 00:07

Ashish1805 force-pushed the ledamping branch from 975ab68 to 7cbdf2b Compare September 29, 2023 15:21

Ashish1805 force-pushed the ledamping branch 2 times, most recently from 2c9b338 to 6cce3ba Compare September 30, 2023 01:27

kcudnik reviewed Sep 30, 2023

View reviewed changes

Ashish1805 marked this pull request as ready for review October 3, 2023 18:37

kcudnik reviewed Oct 4, 2023

View reviewed changes