GiraffaFS / giraffa

Giraffa FileSystem (Slack: giraffa-fs.slack.com)
https://giraffa.ci.cloudbees.com
Apache License 2.0
17 stars 6 forks source link

Refactor INodeManager to remove coprocessor dependecies. #162

Closed shvachko closed 9 years ago

shvachko commented 9 years ago

This is refactoring task to follow up on findings from #158. We just need the Table member in INodeManager. Makes it simpler.

pjeli commented 9 years ago

Seems that a Connection was allowed to slip away in TestLeaseManagement.testMigration -- theres a do/while loop where a Connection is created by never closed.

shvachko commented 9 years ago

I'll include this fix. How do i know it is fixed?

pjeli commented 9 years ago

Please include the following changes in your refactoring:

For point 2, the loss of that extra / in 'hdfs://file1' is causing us to create a NEW FileSystem client because it can't find it in the CACHE and therefore we get a dangling client. This is actually a bug in Hadoop as well.

shvachko commented 9 years ago

Cannot change testHDFSConf.xml. It is exactly as in hdfs, man.

pjeli commented 9 years ago

Then we need to add a replace all "hdfs://file1" to "hdfs:///file1" in the setUp for TestGiraffaCLI, as an additional part to creating testGRFAConf.xml.

pjeli commented 9 years ago

Just tested, adding the following to TestGiraffaCLI.setUp() will achieve the same result:

shvachko commented 9 years ago

But both should work. ://file1 means relative path, while :///file1 is absolute. No?

shvachko commented 9 years ago

Talked to Plamen offline. The problem is that Hadoop treats grfs:// and grfs:/// as different URIs. So CLI test creates two instances of FileSystem - one for each of the URIs, because it cannot find the second in FileSystem cache by the URI. Then the test closes only one cachedinstance, while the other remains in the cache. The same problem of dangling clients exists in HDFS, but nobody cares there.

We should file an HDFS Jira, fix it now for Giraffa by modifying testGRFAConf.xml as Plamen proposed with a comment explaining the condition. Although, let's do it in a separate issue, so that we could revert it explicitly, when HDFS is fixed.

shvachko commented 9 years ago
milandesai commented 9 years ago

Looks good, +1

shvachko commented 9 years ago

About removing ThreadLocal from INodeManager. At least for now we are creating INodeManager whenever we need a connection. So I don't see it needed. More to that, RegionServer has multiple handlers, which being separate thread require their own reference to the nsTable, which is redundant.

shvachko commented 9 years ago

Just committed.