Opened 8 years ago

Closed 8 years ago

#1547 closed task (done)

Review Hive Server

Reported by: cneumuel Owned by: ascheibe
Priority: medium Milestone: HeuristicLab 3.3.6
Component: Hive.Server Version: 3.3.6
Keywords: Cc:

Description

Affected components:

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

Change History (9)

comment:1 Changed 8 years ago by swagner

  • Owner changed from abeham to swagner
  • Status changed from new to assigned
  • Type changed from defect to task

comment:2 Changed 8 years ago by swagner

  • Milestone changed from HeuristicLab 3.3.x Backlog to HeuristicLab 3.3.5
  • Status changed from assigned to reviewing

comment:3 Changed 8 years ago by 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 ...

comment:4 Changed 8 years ago by 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).

comment:5 Changed 8 years ago by 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

comment:6 Changed 8 years ago by swagner

  • Milestone changed from HeuristicLab 3.3.5 to HeuristicLab 3.3.6
  • Version changed from 3.3.4 to branch

comment:7 Changed 8 years ago by ascheibe

  • Owner changed from swagner to ascheibe
  • Status changed from reviewing to readytorelease

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.

comment:8 Changed 8 years ago by abeham

  • Version changed from branch to 3.3.5

comment:9 Changed 8 years ago by swagner

  • Resolution set to done
  • Status changed from readytorelease to closed
  • Version changed from 3.3.5 to 3.3.6
Note: See TracTickets for help on using tickets.