Copilot commented on code in PR #12011:
URL: https://github.com/apache/apisix/pull/12011#discussion_r2844094837
##########
apisix/core/config_etcd.lua:
##########
@@ -807,22 +808,23 @@ local function sync_data(self)
-- avoid space waste
if self.sync_times > 100 then
- local values_original = table.clone(self.values)
- table.clear(self.values)
-
- for i = 1, #values_original do
- local val = values_original[i]
+ local pre = 1
+ local cur = 1
+ table.clear(self.values_hash)
+ log.info("clear stale data in `values_hash` for key: ", key)
+ for _, val in ipairs(self.values) do
if val then
- table.insert(self.values, val)
+ self.values[pre] = val
+ key = short_key(self, val.key)
+ self.values_hash[key] = pre
+ pre = pre + 1
end
- end
- table.clear(self.values_hash)
- log.info("clear stale data in `values_hash` for key: ", key)
+ cur = cur + 1
+ end
- for i = 1, #self.values do
- key = short_key(self, self.values[i].key)
- self.values_hash[key] = i
+ for i = cur - 1, pre, -1 do
Review Comment:
The `cur` variable is only used to track the iteration count and could be
replaced with `#self.values` captured before the loop. For example: `local
original_len = #self.values` before line 815, then use `original_len` instead
of `cur - 1` in line 826. This would eliminate the need for incrementing `cur`
on every iteration and make the code slightly clearer.
##########
apisix/core/config_etcd.lua:
##########
@@ -807,22 +808,23 @@ local function sync_data(self)
-- avoid space waste
if self.sync_times > 100 then
- local values_original = table.clone(self.values)
- table.clear(self.values)
-
- for i = 1, #values_original do
- local val = values_original[i]
+ local pre = 1
+ local cur = 1
+ table.clear(self.values_hash)
+ log.info("clear stale data in `values_hash` for key: ", key)
+ for _, val in ipairs(self.values) do
if val then
- table.insert(self.values, val)
+ self.values[pre] = val
+ key = short_key(self, val.key)
+ self.values_hash[key] = pre
+ pre = pre + 1
end
- end
- table.clear(self.values_hash)
- log.info("clear stale data in `values_hash` for key: ", key)
+ cur = cur + 1
+ end
- for i = 1, #self.values do
- key = short_key(self, self.values[i].key)
- self.values_hash[key] = i
+ for i = cur - 1, pre, -1 do
+ remove_tab(self.values, i)
end
Review Comment:
The refactored array compaction logic (lines 811-828) lacks test coverage.
This is a critical code path that triggers after 100 deletions to clean up the
values array. Consider adding a test that creates multiple items, deletes them
to increment sync_times beyond 100, and verifies the array is correctly
compacted with no data corruption.
##########
apisix/core/config_etcd.lua:
##########
@@ -807,22 +808,23 @@ local function sync_data(self)
-- avoid space waste
if self.sync_times > 100 then
- local values_original = table.clone(self.values)
- table.clear(self.values)
-
- for i = 1, #values_original do
- local val = values_original[i]
+ local pre = 1
+ local cur = 1
Review Comment:
The variable names `pre` and `cur` are unclear and could be more
descriptive. Consider renaming to `write_idx` and `read_idx` respectively to
better convey their purpose in the compaction algorithm. `write_idx` represents
the next position to write a valid element, while `read_idx` tracks the current
position being read from the original array.
##########
apisix/core/config_etcd.lua:
##########
@@ -807,22 +808,23 @@ local function sync_data(self)
-- avoid space waste
if self.sync_times > 100 then
- local values_original = table.clone(self.values)
- table.clear(self.values)
-
- for i = 1, #values_original do
- local val = values_original[i]
+ local pre = 1
+ local cur = 1
+ table.clear(self.values_hash)
+ log.info("clear stale data in `values_hash` for key: ", key)
+ for _, val in ipairs(self.values) do
if val then
- table.insert(self.values, val)
+ self.values[pre] = val
+ key = short_key(self, val.key)
+ self.values_hash[key] = pre
Review Comment:
The variable `key` shadows the outer loop variable declared at line 726.
While this doesn't cause incorrect behavior since `key` is reassigned at the
start of each outer loop iteration, it's poor practice and reduces code
clarity. Consider using a different variable name such as `item_key` or
`short_k` for the inner usage.
```suggestion
local item_key = short_key(self, val.key)
self.values_hash[item_key] = pre
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
