BkTimetable wrong design

[yegor256]

I think that current design of BkTimetable is too complex, that's why the unit test is very difficult to understand. I would refactor it and simplify. I would do it like this:

final class BkTimeable {
  Back back;
  long latency;
  Thread monitor;
  Map<Target, Long> threads; // thread and start time
  public BkTimeable(Back back, long msec) {
    //...
  }
}

Just one ctor, nothing else.

Current unit tests are absolutely wrong. They don't test the class, but instead test its interaction with encapsulated elements. It's against the idea of OOP. Let's remove current tests and create a simple integration test, which will run a server (like it's done in FtCLITest) and make sure the request is interrupted in a few seconds.

[dmzaytsev]

@yegor256 @davvd please assign this task to me

[yegor256]

@davvd assign @dmzaytsev to this task pls

[dmzaytsev]

@yegor256 should I use Thread instance instead ScheduledExecutorService ? something like this

new Thread(new Runnable() {
    @Override
        public void run() {
            while (true) {
                // look for expired threads and interrupt them
               Thread.sleep();
            }
).start();

[davvd]

@yegor256 I will find a developer for the task soon...

[davvd]

@yegor256 milestone set to 1.0 (correct me if I am wrong)

[davvd]

@yegor256 thanks a lot for reporting, 15 mins added to your acc, pmt ID 000-e27c2164

[davvd]

@davvd assign @dmzaytsev to this task pls

@yegor256 OK @dmzaytsev please go ahead, this task is yours

[yegor256]

@dmzaytsev yes, that's how I would do this, with while(true) and TimeUnit.SECONDS.sleep(1L)

[dmzaytsev]

@yegor256 We have some problems when using BkTimeable with BkParallel. BkTimeable catches the current thread, but BkParallel creates a new thread which unknown for BkTimeable. We need to change call order for them.

Also I suggest to break BkTimeable into two classes Watcher and BkTimeable.
So FtCLI code will look something like this

    final Front front = new FtBasic (
                 new BkParallel (
                  new BkTimeable (
                     new Watcher.Base (this.options.maxLatency ()),
                     new BkSafe (new BkBasic (tks))
                  )
                 this.options.threads ()
             )
             this.options.port ()
         );

This will allow to use the single watcher for the whole application if we have several listeners.
What do you think about the suggestion?

[dmzaytsev]

@davvd the PR #216 for the task is ready for review
@yegor256 if you accept the suggestion, I will do it later

[yegor256]

@dmzaytsev I didn't get the idea of watcher... why do we need to make it visible/changeable outside of BkTimeable?

[dmzaytsev]

@yegor256 Idea of watcher is sharing one watcher between many thread catchers. In case if we use multiple instances of Front(listen different ports). But this is rare case. I think it is not required.

[yegor256]

@dmzaytsev yes, it's over-engineering. let's keep it simple

[dmzaytsev]

@yegor256 the PR #216 has been merged. Could you close the task please ? Or let me know why the delivery not accepted

[yegor256]

@dmzaytsev looks perfect now, thanks!

[dmzaytsev]

@yegor256 you are welcome :)

[davvd]

@elenavolokhova please, let us know what do you think about this ticket, according to our QA rules

[elenavolokhova]

@davvd Quality is good.

[davvd]

@davvd Quality is good.

@elenavolokhova thanks for the review

[davvd]

@davvd Quality is good.

@elenavolokhova thank you, good quality is always good :)

[davvd]

@dmzaytsev I added 10 mins to @elenavolokhova (for QA review) in transaction 56854433

Thanks a lot, I just topped your account for 30 mins, transaction ID 56854444, total time was 80 hours and 28 mins

+30 added to your rating, at the moment it is: +1212