LDAP replication FE issues this will just be the bug I check in some of the FE work that I'm doing for LDAP replication while rdayal does the BE work.

What are some of the FE changes that you will be making?

Front End Issues that I'll be starting on, and then handing over to rdayal 1) when offline, in the local autocomplete code, we need to determine if we are set to do LDAP autocomplete, and if so, see if there is a non bogus .mab file for the LDAP server we use for LDAP autocomplete, and autocomplete against it. [we need to leave it open for performance, like we do other local abs. we need to close it when going online, and we could close it if the user changes the prefs to autocomplete against that LDAP addressbook.] 2) when offline, and the user selects an LDAP server, we want to use the .mab file, but it should behave like an LDAP server (blank until we quick search). this is important because for large .mab files, it is expensive to sort and generate all the collation keys. 3) how do we handle LDAP directory deletion? (we need to delete the .mab file also, but see #2, the filename pref may be bogus. see #99124) 4) do we need to "re-generate" the filename pref? (yes, on initial replication we might need to regenerate the filename, if it is bogus). 5) when we change online state, we need to re-root (and re-quick search?) any views to LDAP servers. 6) scenarios where you go offline while a search is in progress. Yes, in this scenario we'll have to first stop the search. (note, there is no way to do this currently, I have a bug to add support for stop.) 7) are we not able to compact mork? I need to implement #1 and #2 soon, so that rdayal can use the AB front end to test his replication backend code.

I have the beginnings of a patch, that when offline LDAP queries turn into local queries. I'll attach it so rdayal and dmose can comment.

if I'm offline, and I'm doing a ldap query, I forward the GetChildCards() to the directory for the local version of that query. note, I've hard coded so all offline ldap queries go to Phonebook.mab. which is my ldap directory. my prefs, would should help explain: user_pref("ldap_2.servers.Phonebook.description", "Phonebook"); user_pref("ldap_2.servers.Phonebook.filename", "Phonebook.mab"); user_pref("ldap_2.servers.Phonebook.replication.lastChangeNumber", 0); user_pref("ldap_2.servers.Phonebook.uri", "ldap://nsdirectory.netscape.com:389/ou=People,dc=netscape,dc=com??sub");

that patch is for the second issue: 2) when offline, and the user selects an LDAP server, we want to use the .mab file, but it should behave like an LDAP server (blank until we quick search). this is important because for large .mab files, it is expensive to sort and generate all the collation keys. I'll go fix the hard coding to Phonebook.mab, so that rdayal can use this patch to test his LDAP replication code. note, it will assume that your filename pref is valid. handling edge cases where it isn't (and it points to the personal address book) can come later. then, I'll go look into the #1 issue, getting ldap autocomplete to work when offline, if we've replicated.

one issue with this patch, is it, like the existing code in nsAbLDAPDirectory::InitiateConnection() and other code around the tree, goes through the prefs directly. this code (like the other code nsAbLDAPDirectory::InitiateConnection) is "less evil" since it is just reading, but this still needs to be addressed. there is an open bug on this issue.

Can we not use DirPrefs for reading the pref as per your suggestion to remove reading the prefs directly ? I am using that in my code too, something like : DIR_Server dirServer ; dirServer.replInfo = nsnull ; dirServer->prefName = nsCRT::strdup(prefName); // LDAP URI without query DIR_GetPrefsForOneServer(dirServer, PR_FALSE, PR_FALSE); // 'dirServer.fileName' is the filename used by replication and required here.

here's a new patch. rdayal's suggestion was very useful. I've fixed that code to use DIR_GetPrefsForOneServer() I've also used it to fix the other place I was reading prefs directly, and I've used it to fix bug #116984 (we no longer have to hard code maxHits to 100)

note, I'm hard coding to my Phonebook.mab, fixing that now.

this patch makes is so when you are offline, you can use the replicated LDAP directory for autocomplete and in the addressbook. I've removed the hard coding.

Comment on attachment 72233 [details] [diff] [review] same patch, fix a clobber build issue Some review comments: NeedToSearchLocalDirectories(): a) why pre-initialize *aNeedToSearch and then overwrite it? NeedToSearchReplicatedDirectories(): b) same as comment a; additionally, I think it should be possible to get rid of enableLDAPAutocomplete entirely and re-use aNeedToSearch SearchReplicatedLDAPDirectories(): c) I think calloc() is preferred to PR_Calloc these days, for performance reasons. PR_Calloc does a memset() to zero out the memory. I recall a discussion where sfraser mentioned that (at least) Mac OS X provides a calloc() which does the zero automagically at page-in time using MMU tricks. Actually, even better might be to change dir_DeleteServerContents to be to DIR_DeleteServerContents (ie have nsDirPrefs export it) and then just put the DIR_Server struct itself on the stack. The same comments apply to GetDIR_Server. d) How about using strdup() instead of nsCRT::strdup()? e) If you condense the localDirectoryURI stuff into a single statement, it'll save an allocation. The same comments apply to GetChildCards, only there it'll save a bunch of allocations. SearchDirectory: f) + if (enableLocalAutocomplete) { rv = SearchDirectory(kAllDirectoryRoot, &searchStrings, results, PR_TRUE); + NS_ASSERTION(NS_SUCCEEDED(rv), "searching all local directories failed"); + } + + if (enableReplicatedLDAPAutocomplete) { + rv = SearchReplicatedLDAPDirectories(prefs, &searchStrings, results, PR_TRUE); + NS_ASSERTION(NS_SUCCEEDED(rv), "searching all replicated LDAP directories failed"); + } Looking at the code in context, it appears that if only replicated-autocomplete fails, nsIAutoCompleteStatus::failed will be returned. But if only local-autocomplete fails, a different status will be returned. I'd vote for returning one of the success codes if either one of the Search*Directories() functions succeeds. g) Having just read through GetPrefForOneServer, it's an incredibly expensive cost to pay for just getting a single preference (a metric buttload of string copies, for one thing). Unless DIR_Server is a being used to delay disk writes and we have to use it to get the most current info, I'd really prefer getting rid of DIR_Server entirely from this patch.

I was looking at nsDirPrefs and see that it does not really give an advantage for reading or writing directory preferences. It does not either have any Observor for the related prefs or any notification for its clients if the prefs changes, there is notifications for the UI when position changes but not really when prefs are changed and saved. Further for it to be used as a central place to access directory prefs it should ideally be implemented as a singleton or as a XPCOM service. I have opened an enhancement bug (milestone future) for clean up of nsDirPrefs, bug # 129326. I feel nsDirPrefs can be only useful for code reuse in case if there is a need to access more than few directory preferences. Seth, your decision here will also be useful for me. thanks.

a) addressed b) addressed c1) addressed d) addressed e) addressed f) addressed c2) > Actually, even better might be to change dir_DeleteServerContents to > be to DIR_DeleteServerContents (ie have nsDirPrefs export it) and then > just put the DIR_Server struct itself on the stack. > The same comments apply to GetDIR_Server. I'm not sure I want to expose more of internals of the DIR_Server. > g) Having just read through GetPrefForOneServer, it's an incredibly > expensive cost to pay for just getting a single preference (a metric > buttload of string copies, for one thing). Unless DIR_Server is a > being used to delay disk writes and we have to use it to get the most > current info, I'd really prefer getting rid of DIR_Server entirely > from this patch. I agree, the existing DIR_Server code is heavy weight. On one of the things it provides is an abstraction on top of prefs so that when we delete servers, all the necessary prefs are delete. It also provides an good place to put the code that generates a unique .mab file name I need to go study the existing code. I'm not against us replacing it with something simpler, scriptable, and lighter weight. now that we have XPCOM, we could do a lot of clean up. (the DIR_Server code, from 4.x, has its own ref counting stuff.) That said, I don't think now is the time to rip it out. When we do come up with a clean interface and implementation, it should be easy to replace this code. I'd rather not write [more] code that just reads prefs directly. I'd rather use DIR_Server code until it is replaced. this is covered by rdayal's new bug: #129326. cc srilatha, as a scriptable DIR_Server like service will affect her, and allow her to clean up a lot of the existing js that reads / writes prefs directly. comments?

talking with dmose privately, he convinced me why it is better to add new (attribute getting) code that uses prefs directly. see http://bugzilla.mozilla.org/show_bug.cgi?id=129326#c2 the way this affects this patch is I'll fix the code to go back to reading prefs directly, and that code will be fixed when 129326 is fixed. note to srilatha and rdayal, if you are writing code to get addressbook attributes, use prefs directly, until 129326. I'm not sure about setting addressbook values, that probably depends on where from (C++ or JS) and what values. it might make sense to still use DIR_Server in certain C++ cases. new patch coming soon.

Comment on attachment 73282 [details] [diff] [review] patch to use read prefs directly. r=dmose

Comment on attachment 73282 [details] [diff] [review] patch to use read prefs directly. sr=bienvenu - there are some tabs, I think, that need cleaning up, and as a style point, return result(s) should be the last parameter(s) in methods (+nsAbAutoCompleteSession::SearchReplicatedLDAPDirectories)

Comment on attachment 73282 [details] [diff] [review] patch to use read prefs directly. a=dbaron for trunk checkin

fixed. I fixed the tabs and fixed bienvenu's style point for the code I added, and the existing code that I modeled my code after. this blocks #59038 (the LDAP replication feature) so noting that.

Add Gary Chan and myself to CC list

Yulian, doesn't this bug fall into your area for testing? It's testing a replicated LDAP directory in both Online and Offline state. If so, please reassign QA to yourself. Thanks

Reassign QA Contact to myself.

Verified with 20020410 trunk builds on various platforms.