Review Hive Server

[cneumuel]

Affected components:

• HeuristicLab.Services.Hive-3.4
• HeuristicLab.Services.Hive.Common-3.4
• HeuristicLab.Services.Hive.DataAccess-3.4

[swagner]

Review comments:

• General:
  • License headers are missing in some source files.
  • The version number of all Hive plugins should be decremented to 3.3.4.* before merging the plugins back into the trunk.
  • AssemblyDescriptions should be given in all AssemblyInfo files.
  • using blocks should not be located inside namespaces but at the beginning of a source file.

• HeuristicLab.Services.Hive.DataAccess:
  • TransactionManager should be static.
  • DaoException is only thrown in HiveDao.DeleteSlaveGroup. I would recommend to remove DaoException and to throw an InvalidOperationException instead. In general I am not a fan of implementing custom exceptions, if there are reasonable exceptions provided in the .NET framework which can be used instead.
  • I do not really like that the Hive data access plugin has to have a dependency on the Hive common plugin. In my opinion, the data access plugin should not know anything about DTOs and about how to convert database entity objects into DTOs. This logic should be located somewhere else (most likely in the main Hive service plugin). The data access plugin should only deal with accessing the database. What is happening with the database entity objects should be in the responsibility of those who use the Hive data access plugin.
  • The TimeSpanExtensions extension method should not be in HiveDao.cs but should be implemented in a separate file. However, generally I am not sure, if we really need the TimeSpanExtensions extension method. It is used only once (Hive.Dao.GetUserStatistics) and in that case, a simple Aggregate call should be enough.
  • The file prepareHiveDatabase.sql should be renamed to Prepare Hive Database.sql.
  • The folder Tools should also contain a SQL script Initialize Hive Database.sql for initializing a new and empty database with all required tables, etc.

• HeuristicLab.Services.Hive.Common:
  • ... to do ...

• HeuristicLab.Services.Hive:
  • ... to do ...

• HeuristicLab.Services.Hive.Tests:
  • ... to do ...

[cneumuel]

swagner:

I do not really like that the Hive data access plugin has to have a dependency on the Hive common plugin. In my opinion, the data access plugin should not know anything about DTOs and about how to convert database entity objects into DTOs. This logic should be located somewhere else (most likely in the main Hive service plugin). The data access plugin should only deal with accessing the database. What is happening with the database entity objects should be in the responsibility of those who use the Hive data access plugin.

Well, the idea is that the DataAccess plugin should be exchangeable. So if the DataAccess layer would be replaced with a new one (e.g. one that uses Entitiy Framework), the business logic should not be affected by this change. When moving the Convert class to the business logic, it needs to know details about the DataAccess.

However, to be honest, right now the business logic has a dependency to the LinqToSql entity objects of the DataAccess layer - this is to enable easy where-statements (predicates on entities as delegates).

[cneumuel]

r6431 - applied some review comments

General:

• changed Log to ThreadSafeLog
• added license information to all files
• added assembly descriptions
• using blocks before namespace

HeuristicLab.Services.Hive.DataAccess:

• made TransactionManager static
• removed DaoException
• removed TimeSpanExtensions
• renamed prepareHiveDatabase.sql should be renamed to Prepare Hive Database.sql
• created Initialize Hive Database.sql

[ascheibe]

I'm setting this ticket to readytorelease because Hive is now in trunk. All the above mentioned review comments have been integrated. The Hive integration in trunk is tracked with ticket #1672.