Forgot to do "Reply to All".<br><br><div class="gmail_quote">On Wed, Jul 14, 2010 at 4:59 PM, Vishesh Handa <span dir="ltr"><<a href="mailto:handa.vish@gmail.com">handa.vish@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">Hey Sebastian<br><br><div class="gmail_quote"><div class="im">On Wed, Jul 14, 2010 at 2:59 PM, Sebastian Trüg <span dir="ltr"><<a href="mailto:trueg@kde.org" target="_blank">trueg@kde.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
Hi Vishesh,<br>
<br>
great work. Some comments:<br>
<div><br>
On 07/13/2010 10:21 PM, Vishesh Handa wrote:<br>
> The current patch checks for blank Nodes in the object / subject of the<br>
> file's metadata, and tries to find them or creates them if not present.<br>
> The patch reverts to a the original behavior if any of the additionally<br>
> generated metadata ( contacts, albums) contain any blank nodes. in order<br>
> to fix this, a full blown dependency resolution algorithm would be<br>
> required. I don't think that it is currently required.<br>
<br>
</div>Could you elaborate on this please. I do not follow exactly. The problem<br>
would be if there was for example a contact that was related to another<br>
contact? If so, I doubt any strigi analyzer does create such data atm.<br>
<div><br></div></blockquote></div><div> Yes, the problem would arise if one contact is related to another. AFAIK no strigi analyzer creates such data, and even if one of them does create such data. It would land up creating the resources which is what the earlier IndexWriter did.<br>
</div><div class="im"><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div>
> The patch also creates a different graph ( discardable ) for each<br>
> individual resource.<br>
<br>
</div>very good.<br>
<br>
> *Problem not fixed :*<br>
<div>> This will only work on newly indexed files and does not affect the files<br>
> which have already been indexed. We'll need some kind of merger to do<br>
> that. It's a lot simpler to just re-index the files, but I don't think<br>
> the end users would like that.<br>
<br>
</div>I am still for reindexing files based on mimetype whenever there are new<br>
features available for that mimetype.<br></blockquote></div><div><br>Same here, but I doubt the users would be. I was trying to implement resource merging yesterday and I don't think it is difficult, the only problem is that I couldn't find any fast way to check if resources need to be merged. ( Maybe in 1-2 queries, any idea? ) The actual merging would be somewhat slow, but that's okay. It would be done in a separate thread and would only be done once or twice.<br>
<br></div><div class="im"><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
> *A New Problem :<br>
<div>> *Since the additional metadata now has it's own graph. It will not be<br>
> deleted if the file is deleted. We need some kind of cleaner which<br>
> cleans resources which are no longer in use.<br>
<br>
</div>I see 2 solutions:<br>
1. Do it in a cleaner service the same way we clean up unused metadata<br>
graphs and the like.<br>
2. Do it when deleting the files' metadata in the strigi service and in<br>
the filewatch service using shared code.<br></blockquote></div><div><br>The second option would be easier, but I'm not too keen on slowing down the metadatamover service or the strigi service. They have enough to deal with as it is. For now, we can just let the additional resources be.<br>
</div><div class="im"><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
<br>
And now the code review:<br>
1. Please add comments on all methods as if it were public API. While<br>
this is not required and I do not do it all the time it will simplify<br>
working on the code together.<br></blockquote></div><div><br>Okay<br> </div><div class="im"><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
2. the variable names are not very descriptive: m_queue and m_stack. :P<br></blockquote></div><div><br>I've renamed m_queue to m_updateQueue, but I'm not sure what I should do with m_stack. I could maybe call it feeder stack? Do you have suggestions? I have however, add comments that say what the variable does.<br>
<br></div><div class="im"><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
3. I think we could do most of generateGraph using Soprano::NRLModel<br>
4. createNode() is not descriptive enough. It is not clear what it does<br>
when you see it used in code. Better call it something like<br>
blankNodeFromIdentifier or something like that.<br></blockquote></div><div><br>I've renamed it to createBlankOrResourceNode() as that is what it does.<br> <br></div><div class="im"><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
5. the feeder is started twice (from a conceptual point of view): once<br>
after constructing it and once in startAnalysis. IMHO you could simply<br>
start the feeder in its constructor and thus, hide the threading issue<br>
away more.<br></blockquote></div><div><br>Done. :)<br> <br></div><div class="im"><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
6. "// Handle the feeder" is not the greatest comment as it only<br>
clutters the code without adding any new information. The fact that<br>
something is being done with the feeder is obvious. :)<br></blockquote></div><div><br>Uhh. Yea, stupid comment. I've removed it for now.<br> <br></div><div class="im"><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
7. the name StrigiFeeder is a bit misleading. The class does not feed<br>
anything into Strigi. It does the exact opposite. So maybe NepomukFeeder<br>
would be better... i dont know.<br></blockquote></div><div><br>I found the name NepomukFeeder rather generic. I've named it to NepomukIndexFeeder for now. :-/<br> <br></div><div class="im"><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
8. You did it again: a static method named "toSparql". Please do not do<br>
that. :)<br></blockquote></div><div><br>But this time I added documentation as to what kind of query it creates. I understand that these kind of function names are bad, but I can't think of any alternative. Could you please suggest some name?<br>
</div><div class="im"><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
9. Why don't you store the set of statements as ResourceHash right away?<br>
<br></blockquote></div><div>Yes. I thought about this for quite some time. I actually made a list of pros and cons. In the end I decided to go with this approach as it required less modification of code ( less error prone ) and is technically more portable. Plus, ResourceStruct is a lousy name and I didn't really want to make the IndexWriter depend on it.<br>
<br>But I think we should think about standardizing ResourceStruct. I use it a lot in BackupSync and I use it in the API Artem wants ( which is difficult to design ). Plus it's really convenient! <br> </div><div class="im">
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
As you can see I have no "real" comments since IMHO you did a great job.<br>
So please go ahead and commit that (maybe with some changes based on my<br>
comments) to trunk. Then testing can commence. :)<br></blockquote></div><div><br>Are you sure? Just say "Yes' and I'll commit it. <br></div><div class="im"><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
Cheers,<br>
Sebastian<br>
_______________________________________________<br>
Nepomuk mailing list<br>
<a href="mailto:Nepomuk@kde.org" target="_blank">Nepomuk@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/nepomuk" target="_blank">https://mail.kde.org/mailman/listinfo/nepomuk</a><br>
</blockquote></div></div><br>
</blockquote></div><br>