bzp2010 commented on code in PR #12214:
URL: https://github.com/apache/apisix/pull/12214#discussion_r2094929685
##########
apisix/core/config_yaml.lua:
##########
@@ -158,29 +149,59 @@ local function sync_data(self)
return nil, "missing 'key' arguments"
end
- if not apisix_yaml_mtime then
- log.warn("wait for more time")
- return nil, "failed to read local file " .. apisix_yaml_path
+ local conf_version
+ if is_use_admin_api() then
+ conf_version = apisix_yaml[self.conf_version_key] or 0
+ else
+ if not apisix_yaml_mtime then
+ log.warn("wait for more time")
+ return nil, "failed to read local file " .. apisix_yaml_path
+ end
+ conf_version = apisix_yaml_mtime
end
- if self.conf_version == apisix_yaml_mtime then
+ if not conf_version or conf_version == self.conf_version then
return true
end
local items = apisix_yaml[self.key]
- log.info(self.key, " items: ", json.delay_encode(items))
if not items then
self.values = new_tab(8, 0)
self.values_hash = new_tab(0, 8)
- self.conf_version = apisix_yaml_mtime
+ self.conf_version = conf_version
return true
end
- if self.values then
- for _, item in ipairs(self.values) do
- config_util.fire_all_clean_handlers(item)
+ if self.values and #self.values > 0 then
+ if is_use_admin_api() then
+ -- used to delete values that do not exist in the new list.
+ -- only when using modifiedIndex, old values need to be retained.
+ -- If modifiedIndex changes, old values need to be removed and
cleaned up.
+ local exist_modifiedIndex_items = {}
+ for _, item in ipairs(items) do
+ if item.modifiedIndex then
+ exist_modifiedIndex_items[tostring(item.id)] = true
+ end
+ end
+
+ local new_values = new_tab(8, 0)
+ self.values_hash = new_tab(0, 8)
+ for _, item in ipairs(self.values) do
+ local id = item.value.id
+ if not exist_modifiedIndex_items[id] then
+ config_util.fire_all_clean_handlers(item)
+ else
Review Comment:
A correct process is:
1. Check which resources (of ids) existed in the old configuration set but
not in the new configuration set, which means that these resources need to be
removed and cleaned up.
2. check which resources exist in both the old and new configuration sets,
but have inconsistent modifiedIndex, meaning they need to be updated.
3. check for resources that exist in the new configuration set but not in
the old, meaning they need to be created.
Where 1, should be realized by the resource id, not the modifiedIndex, they
have nothing to do with each other. The actual situation is that if for a given
resource id, if the id is changed to some other value, it means deleting a
resource represented by the old id and creating a resource represented by the
new id, which would be the simplest and most reliable pattern.
ADC's "differ" is the exact implementation of this idea. Except that it
allows a comparison range beyond modifiedIndex, and you can check its code and
implement a simple equivalent.
https://github.com/api7/adc/blob/main/apps/cli/src/differ/differv3.ts#L308-L639
##########
apisix/core/config_yaml.lua:
##########
@@ -256,12 +281,26 @@ local function sync_data(self)
end
if data_valid then
- insert_tab(self.values, conf_item)
- local item_id = conf_item.value.id or self.key .. "#" .. id
- item_id = tostring(item_id)
- self.values_hash[item_id] = #self.values
- conf_item.value.id = item_id
- conf_item.clean_handlers = {}
+ local item_id = tostring(id)
+ local pre_index = self.values_hash[item_id]
+ if pre_index then
+ -- remove the old item
+ local pre_val = self.values[pre_index]
+ if pre_val and
+ (not pre_val.modifiedIndex or pre_val.modifiedIndex ~=
modifiedIndex) then
+ log.error("fire all clean handlers for ", self.key, "
id: ", id,
+ " modifiedIndex: ", modifiedIndex)
+ config_util.fire_all_clean_handlers(pre_val)
+ self.values[pre_index] = conf_item
+ conf_item.value.id = item_id
+ conf_item.clean_handlers = {}
+ end
Review Comment:
Since self.values has been overwritten with a new table above (different
pointer address), what is this dealing with?
##########
apisix/core/config_yaml.lua:
##########
@@ -158,29 +149,59 @@ local function sync_data(self)
return nil, "missing 'key' arguments"
end
- if not apisix_yaml_mtime then
- log.warn("wait for more time")
- return nil, "failed to read local file " .. apisix_yaml_path
+ local conf_version
+ if is_use_admin_api() then
+ conf_version = apisix_yaml[self.conf_version_key] or 0
+ else
+ if not apisix_yaml_mtime then
+ log.warn("wait for more time")
+ return nil, "failed to read local file " .. apisix_yaml_path
+ end
+ conf_version = apisix_yaml_mtime
end
- if self.conf_version == apisix_yaml_mtime then
+ if not conf_version or conf_version == self.conf_version then
return true
end
local items = apisix_yaml[self.key]
- log.info(self.key, " items: ", json.delay_encode(items))
if not items then
self.values = new_tab(8, 0)
self.values_hash = new_tab(0, 8)
- self.conf_version = apisix_yaml_mtime
+ self.conf_version = conf_version
return true
end
- if self.values then
- for _, item in ipairs(self.values) do
- config_util.fire_all_clean_handlers(item)
+ if self.values and #self.values > 0 then
+ if is_use_admin_api() then
+ -- used to delete values that do not exist in the new list.
+ -- only when using modifiedIndex, old values need to be retained.
+ -- If modifiedIndex changes, old values need to be removed and
cleaned up.
+ local exist_modifiedIndex_items = {}
+ for _, item in ipairs(items) do
+ if item.modifiedIndex then
+ exist_modifiedIndex_items[tostring(item.id)] = true
+ end
+ end
+
+ local new_values = new_tab(8, 0)
+ self.values_hash = new_tab(0, 8)
+ for _, item in ipairs(self.values) do
+ local id = item.value.id
+ if not exist_modifiedIndex_items[id] then
+ config_util.fire_all_clean_handlers(item)
+ else
+ insert_tab(new_values, item)
+ self.values_hash[id] = #new_values
+ end
+ end
+ self.values = new_values
Review Comment:
Why is the new value set here? Shouldn't it be set after the schema check
below?
##########
apisix/core/config_yaml.lua:
##########
@@ -158,29 +149,59 @@ local function sync_data(self)
return nil, "missing 'key' arguments"
end
- if not apisix_yaml_mtime then
- log.warn("wait for more time")
- return nil, "failed to read local file " .. apisix_yaml_path
+ local conf_version
+ if is_use_admin_api() then
+ conf_version = apisix_yaml[self.conf_version_key] or 0
+ else
+ if not apisix_yaml_mtime then
+ log.warn("wait for more time")
+ return nil, "failed to read local file " .. apisix_yaml_path
+ end
+ conf_version = apisix_yaml_mtime
end
- if self.conf_version == apisix_yaml_mtime then
+ if not conf_version or conf_version == self.conf_version then
return true
end
local items = apisix_yaml[self.key]
- log.info(self.key, " items: ", json.delay_encode(items))
if not items then
self.values = new_tab(8, 0)
self.values_hash = new_tab(0, 8)
- self.conf_version = apisix_yaml_mtime
+ self.conf_version = conf_version
return true
end
- if self.values then
- for _, item in ipairs(self.values) do
- config_util.fire_all_clean_handlers(item)
+ if self.values and #self.values > 0 then
+ if is_use_admin_api() then
+ -- used to delete values that do not exist in the new list.
+ -- only when using modifiedIndex, old values need to be retained.
+ -- If modifiedIndex changes, old values need to be removed and
cleaned up.
+ local exist_modifiedIndex_items = {}
+ for _, item in ipairs(items) do
+ if item.modifiedIndex then
+ exist_modifiedIndex_items[tostring(item.id)] = true
+ end
+ end
+
+ local new_values = new_tab(8, 0)
+ self.values_hash = new_tab(0, 8)
+ for _, item in ipairs(self.values) do
+ local id = item.value.id
+ if not exist_modifiedIndex_items[id] then
+ config_util.fire_all_clean_handlers(item)
+ else
Review Comment:
Why does resource cleanup use `modifiedIndex` to determine if a resource
needs to be deleted?
They don't seem to be related, it should be based on the resource id, such
as the route id or consumer username.
--
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: notifications-unsubscr...@apisix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
