Update patch set 3
Patch Set 3:
(10 comments)
Patch-set: 3
CC: Gerrit User 1000004 <1000004@035e6965-6537-41bd-912c-053f3cf69326>
Attention: {"person_ident":"Gerrit User 1000005 \u003c1000005@035e6965-6537-41bd-912c-053f3cf69326\u003e","operation":"ADD","reason":"\u003cGERRIT_ACCOUNT_1000004\u003e replied on the change"}
This commit is contained in:
Gerrit User 1000004
Gerrit Code Review
parent
e0bfdcb308
commit
dd9ca2741a
|
|
@ -0,0 +1,216 @@
|
|||
{
|
||||
"comments": [
|
||||
{
|
||||
"unresolved": true,
|
||||
"key": {
|
||||
"uuid": "f2fbe5f3_0738a72d",
|
||||
"filename": "/COMMIT_MSG",
|
||||
"patchSetId": 3
|
||||
},
|
||||
"lineNbr": 11,
|
||||
"author": {
|
||||
"id": 1000004
|
||||
},
|
||||
"writtenOn": "2024-03-22T07:52:03Z",
|
||||
"side": 1,
|
||||
"message": "nftables rules, not netfilter rules.",
|
||||
"range": {
|
||||
"startLine": 11,
|
||||
"startChar": 33,
|
||||
"endLine": 11,
|
||||
"endChar": 42
|
||||
},
|
||||
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
|
||||
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
|
||||
},
|
||||
{
|
||||
"unresolved": false,
|
||||
"key": {
|
||||
"uuid": "fb8b29e2_4bebc7ab",
|
||||
"filename": "/PATCHSET_LEVEL",
|
||||
"patchSetId": 3
|
||||
},
|
||||
"lineNbr": 0,
|
||||
"author": {
|
||||
"id": 1000004
|
||||
},
|
||||
"writtenOn": "2024-03-22T07:52:03Z",
|
||||
"side": 1,
|
||||
"message": "What I am missing here is doing all those nft operations (which take quite some time in blocking way to complete) in a separate thread. I distinctly remember that during the existing discussion, I originally proposed a separate process, and Pau then suggested to use a separate thread. There was no discussion of doing it in the main thread, which will inevitably significantly slow down processing of a hnbgw under high load.\n\nIn my opinion, that separate thread should take care fo rule insertion, rule deletion and counter reading.\n\nThe 1s default interval is way too low and costly. I\u0027d go for certainly no less than 10s; in large setups maybe even less than that.",
|
||||
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
|
||||
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
|
||||
},
|
||||
{
|
||||
"unresolved": true,
|
||||
"key": {
|
||||
"uuid": "f81ea47b_cbe1f041",
|
||||
"filename": "include/osmocom/hnbgw/hnbgw.h",
|
||||
"patchSetId": 3
|
||||
},
|
||||
"lineNbr": 140,
|
||||
"author": {
|
||||
"id": 1000004
|
||||
},
|
||||
"writtenOn": "2024-03-22T07:52:03Z",
|
||||
"side": 1,
|
||||
"message": "in mobile communications, we alwasy speak of Uplink and Downlink, whcih are uniquely defined and well-understood terms, in that they always are viewing it from the UE perspective.\n\nAlso, all of the counters above are using _UL and _DL suffix, so it would be good to be consistent with the other contents in using that abbreviation and using it at the end.",
|
||||
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
|
||||
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
|
||||
},
|
||||
{
|
||||
"unresolved": true,
|
||||
"key": {
|
||||
"uuid": "121895da_559c2c47",
|
||||
"filename": "src/osmo-hnbgw/hnbgw.c",
|
||||
"patchSetId": 3
|
||||
},
|
||||
"lineNbr": 463,
|
||||
"author": {
|
||||
"id": 1000004
|
||||
},
|
||||
"writtenOn": "2024-03-22T07:52:03Z",
|
||||
"side": 1,
|
||||
"message": "none of the other counters have a \".\" in their description. It\u0027s not a full sentence anyway.",
|
||||
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
|
||||
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
|
||||
},
|
||||
{
|
||||
"unresolved": true,
|
||||
"key": {
|
||||
"uuid": "28892d36_856cfca9",
|
||||
"filename": "src/osmo-hnbgw/hnbgw.c",
|
||||
"patchSetId": 3
|
||||
},
|
||||
"lineNbr": 466,
|
||||
"author": {
|
||||
"id": 1000004
|
||||
},
|
||||
"writtenOn": "2024-03-22T07:52:03Z",
|
||||
"side": 1,
|
||||
"message": "why is it \"gtpbytes\" and not \"bytes\"?",
|
||||
"range": {
|
||||
"startLine": 466,
|
||||
"startChar": 11,
|
||||
"endLine": 466,
|
||||
"endChar": 19
|
||||
},
|
||||
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
|
||||
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
|
||||
},
|
||||
{
|
||||
"unresolved": true,
|
||||
"key": {
|
||||
"uuid": "3640b18f_331b2f25",
|
||||
"filename": "src/osmo-hnbgw/hnbgw.c",
|
||||
"patchSetId": 3
|
||||
},
|
||||
"lineNbr": 470,
|
||||
"author": {
|
||||
"id": 1000004
|
||||
},
|
||||
"writtenOn": "2024-03-22T07:52:03Z",
|
||||
"side": 1,
|
||||
"message": "why is the \"ul\" in the middl,e while all other coutners above have the ul/dl at the end? That\u0027s inconsistent to the user.",
|
||||
"range": {
|
||||
"startLine": 470,
|
||||
"startChar": 3,
|
||||
"endLine": 470,
|
||||
"endChar": 18
|
||||
},
|
||||
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
|
||||
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
|
||||
},
|
||||
{
|
||||
"unresolved": true,
|
||||
"key": {
|
||||
"uuid": "38dd43bc_4f44ca98",
|
||||
"filename": "src/osmo-hnbgw/hnbgw.c",
|
||||
"patchSetId": 3
|
||||
},
|
||||
"lineNbr": 911,
|
||||
"author": {
|
||||
"id": 1000004
|
||||
},
|
||||
"writtenOn": "2024-03-22T07:52:03Z",
|
||||
"side": 1,
|
||||
"message": "s/netfilter/nftables/. Mentions \"counters\" twice?",
|
||||
"range": {
|
||||
"startLine": 911,
|
||||
"startChar": 18,
|
||||
"endLine": 911,
|
||||
"endChar": 27
|
||||
},
|
||||
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
|
||||
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
|
||||
},
|
||||
{
|
||||
"unresolved": true,
|
||||
"key": {
|
||||
"uuid": "90d48288_a8de85e9",
|
||||
"filename": "src/osmo-hnbgw/hnbgw_vty.c",
|
||||
"patchSetId": 3
|
||||
},
|
||||
"lineNbr": 746,
|
||||
"author": {
|
||||
"id": 1000004
|
||||
},
|
||||
"writtenOn": "2024-03-22T07:52:03Z",
|
||||
"side": 1,
|
||||
"message": "I think the user doesn\u0027t care if some counters are implemented via nftables or in another way. The user cares about per-hnb counters. If we ever decide to implement those counters in a different way, this command would not exist or be very hard to emulate, and have nothing to do with nft/nftables.\n\nIf this is for debugging only, it should at least be HIDDEN so it\u0027s not documented. Also, it might be worth removing it completely, especially if all of the nft handling moves to a separate thread, as it was originally discussed [which likely makes this command more difficult]",
|
||||
"range": {
|
||||
"startLine": 746,
|
||||
"startChar": 7,
|
||||
"endLine": 746,
|
||||
"endChar": 16
|
||||
},
|
||||
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
|
||||
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
|
||||
},
|
||||
{
|
||||
"unresolved": true,
|
||||
"key": {
|
||||
"uuid": "34c84481_a334b52b",
|
||||
"filename": "src/osmo-hnbgw/nft_kpi.c",
|
||||
"patchSetId": 3
|
||||
},
|
||||
"lineNbr": 120,
|
||||
"author": {
|
||||
"id": 1000004
|
||||
},
|
||||
"writtenOn": "2024-03-22T07:52:03Z",
|
||||
"side": 1,
|
||||
"message": "why are we adding a new chain for each and every hnb? Isn\u0027t a rule per hnb (per direction) sufficient?\n\nCounters can be named, and rules can contain a comment field, in case you need some identifier.\n\nchains usually make sense if you have many rules attached to one filter criteria, like first match on the host, then jump to a chain, then execute a list of rules for all the packets matching that host. This is not the case here, as you unconditionally jump to each chain for each packet (no matter which destination), and then you only have a single rule in each chain.",
|
||||
"range": {
|
||||
"startLine": 120,
|
||||
"startChar": 10,
|
||||
"endLine": 120,
|
||||
"endChar": 13
|
||||
},
|
||||
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
|
||||
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
|
||||
},
|
||||
{
|
||||
"unresolved": true,
|
||||
"key": {
|
||||
"uuid": "eee07875_9f8d81ff",
|
||||
"filename": "src/osmo-hnbgw/nft_kpi.c",
|
||||
"patchSetId": 3
|
||||
},
|
||||
"lineNbr": 249,
|
||||
"author": {
|
||||
"id": 1000004
|
||||
},
|
||||
"writtenOn": "2024-03-22T07:52:03Z",
|
||||
"side": 1,
|
||||
"message": "If theo output is json, please let\u0027s not hand-roll our own JSON parser here but us an existing json parser library. The only argument for rolling our own would be that there is a proven benachmark that in our use case no common exisitng json-parser is anywhere near as fast as the hand-rolled one. And in that case, we\u0027d need a benchmark and some kind of test suite to validate our own parser...",
|
||||
"range": {
|
||||
"startLine": 249,
|
||||
"startChar": 39,
|
||||
"endLine": 249,
|
||||
"endChar": 45
|
||||
},
|
||||
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
|
||||
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
|
||||
}
|
||||
]
|
||||
}
|
||||
Loading…
Reference in New Issue