Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Owners and editors should be stored only in database, not in config #49

Closed
ikedas opened this issue Sep 4, 2017 · 13 comments
Closed

Owners and editors should be stored only in database, not in config #49

ikedas opened this issue Sep 4, 2017 · 13 comments

Comments

@ikedas
Copy link
Member

ikedas commented Sep 4, 2017

List administrators (owners and editors) are stored both in database and list config file. Duplicate information cause complexity of the code, and synchronization between both occasionally fails.

One of adequate changes seems deprecating owner/editor fields in list config file. Even doing this, all information will be kept in database.

@ikedas ikedas added the design label Sep 4, 2017
@eiro
Copy link

eiro commented Sep 4, 2017

hello soji,

we spoke about it during the hackathon and we really don't want to remove such a convenient way to configure sympa so:

  • yes: the database must be the reference for the sympa API but
  • yes: the files should be the reference for the administrator (so he can commit the configuration)

from my perspective: everything should be CRUDable from flat files because it's a very simple way to do tasks.

regards

@dverdin
Copy link
Contributor

dverdin commented Sep 4, 2017

Hi soji,
I agree with Marc. Flat files are very useful for admins and can be versioned.
In addition, I would say that owners/editors are also data frequently parsed in list families. So excluding them from the config file would require to create exceptions for these parameters in the Family code.

I'm feeling that we would gain more long term benefits in strenghtening the current synchronization mechanims between database and files rather than in creating such exceptions.

Cheers!

@ikedas
Copy link
Member Author

ikedas commented Sep 4, 2017

@eiro, @dverdin, thanks for response.

I love flat file, too, but its fault is stability. It is essentially the serialized copy of data structure:

  • It is useless unless it is loaded into the memory.
  • It is not able to be shared among multiple processes or hosts with sufficient accuracy.

In short, flat file stored in disk is not suitable for dynamically changed data (remember that admin info can always be changed by inclusion from data source. ...It is also the case of subscriber info).

Database can do such work without additional construct. ...In other words, if text file alone worked well, database was not wanted.

That's why I suggested dropping text config rather than database.


Addition:

Essentially, admins are loaded from config file only once: When the list was created. Afterward, Sympa querys admins using database, but never read them from config file directly.

Thus, synching (writing back) between database and config file is not necessary. It is possible that initial admins may be given in other ways (data source such as include_file, command line parameter etc.).

@dverdin
Copy link
Contributor

dverdin commented Nov 29, 2017

There is a mechanism in Sympa which tests the age of config file when loading a list. if the config file is newer than the last time it was accessed, it is reloaded. Simple yet effective. Did this mechanism disappear?

@agouaux
Copy link

agouaux commented Nov 29, 2017

Well, as a customer, I think this is a bad idea. As documented in #11 this all worked before upgrading to 6.2(.16?) To adopt this approach means that whoever broke the code is unable to fix what they broke, and that seems to be a poor design decision. :-)

Since I'm not a fan of complaining about something without putting in some effort to help rectify the problem, I had hoped to debug this problem a bit more. Alas, time has been tight. Maybe during the Winter break, since we won't be traveling at all, I can spend a couple of days digging into #11 a bit more to at least help pinpoint where the problem I'm seeing is arising.

@ikedas
Copy link
Member Author

ikedas commented Nov 30, 2017

Well, as a customer, I think this is a bad idea. As documented in #11 this all worked before upgrading to 6.2(.16?) (...)

There are no changes related to this proposal before 6.2.16. Of course the bug you experienced is not due to future changes.

@ikedas
Copy link
Member Author

ikedas commented Feb 1, 2018

There is a mechanism in Sympa which tests the age of config file when loading a list. if the config file is newer than the last time it was accessed, it is reloaded. Simple yet effective. Did this mechanism disappear?

I think this may disappear.

See issue #11. Recently, Sympa refers admin information on database, information in config file is no more than duplicate of it, and actually cause problem.

In the first place, there are no config parameter directly defining list members and Sympa gets along well (user_data_source include2 option has been deprecated long a while ago). I think owner/editor may be alike.

@agouaux
Copy link

agouaux commented Feb 1, 2018

Question: if having admins in both the config file and db is such a problem, then why did the problems in #11 only occur after upgrading Sympa to 6.2.18? We have been using Sympa for many years, and the problems in #11 are only recent. I believe they coincided with the implementation of the cache.

Though, not saying having admins only in db is necessarily a bad thing. It will certainly reduce the need to stat the filesystem so frequently, and that in turn along with the cache (once all issues of UI interactions are resolved), might even improve performance. As with all things, there are trade-offs.

(Ironically, we actually have all subscribers of our auto-populated lists in files too via include_file. We have been using this file importer for a long time. However, perhaps I should explore the possibility of using an external db.)

@agouaux
Copy link

agouaux commented Feb 1, 2018

There is a mechanism in Sympa which tests the age of config file when loading a list. if the config file is newer than the last time it was accessed, it is reloaded. Simple yet effective. Did this mechanism disappear?

I can say that while debugging the interactions of adding an owner and editor at the same time (#11), the problem with this approach is that the file stat isn't fast enough when using the Web UI. That is, the results from stat are not granular enough, and the caching exasperated the problem. Perhaps separating list owners and editors to have separate stat files might mitigate that, but then stating the filesystem twice as much, and I imagine that would have at least some impact on performance.....

@racke
Copy link
Contributor

racke commented Feb 1, 2018

Yes, these problems coincided with the cache implementation, which was really wretched. It makes sense to cache things for display, but not for the administration (where you change data). Also way to deeply ingrained into the code.

Also it not exactly easy to deal with two places which can cause changes - but if you want to do that better use Linux features like notify on file changes rather than check stat from your code.

@ikedas
Copy link
Member Author

ikedas commented Apr 30, 2018

Proposed change was done as a part of fix to #11. This issue is closed.

@ikedas ikedas closed this as completed Apr 30, 2018
@dverdin
Copy link
Contributor

dverdin commented May 15, 2018

Er... I think it is a really bad idea to integrate this to new Sympa release.
A lot of people use owners and moderators in files. Anyone using families for example, but alos all the people using scripts to edit files;
It is also very useful for administration debugging: to know at which date an owner was changed only by checking the config files version.
Frankly, integrating this featre right now is a suicide. I, for one, will never install this in production.
Please consider removing this from 6.2.32.

@dverdin dverdin reopened this May 15, 2018
@ikedas
Copy link
Member Author

ikedas commented May 16, 2018

Hi @dverdin,
Please show evidence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants