Gpars removed #253

Merged
merged 9 commits into from Oct 17, 2016

Projects

In Review in Release 0.8.8

4 participants

@hendrikebbers
Member
hendrikebbers commented Aug 30, 2016 edited

This PR removes the dependency to GParse in all modules without the OD event bus. Since the event bus will be refactored (see #127) this dependency to Parse will be removed in future


This change is Reviewable

hendrikebbers added some commits Aug 29, 2016
@hendrikebbers hendrikebbers Sparse removed part 1 7924c6a
@hendrikebbers hendrikebbers Gparse removed part 1 2e7de98
@hendrikebbers hendrikebbers Gparse removed part 3 14766aa
@hendrikebbers hendrikebbers Logging f606030
@hendrikebbers hendrikebbers Blocking Batcher 8813fce
@hendrikebbers hendrikebbers changes b592828
@hendrikebbers hendrikebbers revert commands b1c3242
@hendrikebbers hendrikebbers Batcher working
13d5d46
@hendrikebbers hendrikebbers added this to the 0.8.8 milestone Aug 30, 2016
@hendrikebbers hendrikebbers referenced this pull request Aug 30, 2016
Closed

Remove GParse #252

@coveralls

Coverage Status

Coverage increased (+0.08%) to 69.104% when pulling 13d5d46 on gpars-removed into 1cd72c3 on master.

@blackdrag

I cannot tell in total if the threading changes you did are a suitable replacement of the old threading behaviour. On the first look the changes you did seem to realize a sensible logic though

+ Condition emptyCondition = queueLock.newCondition();
+
+ CommandBatcher() {
+ this.waitingBatches = new DataflowQueue<List<CommandAndHandler>>() {
@blackdrag
blackdrag Sep 15, 2016

why not move this to its own class and base the implementation on one of the Java Queues?

- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
@blackdrag
blackdrag Sep 15, 2016

still don't understand why you always want to remove license information like this

- LOG.trace("processing {}", command);
- result.addAll(serverConnector.receive(command));// there is no need for encoding since we are in-memory
+ LOGGER.trace("processing {}", command);
+ ((LinkedList<Command>) result).addAll(serverConnector.receive(command));// there is no need for encoding since we are in-memory
@blackdrag
blackdrag Sep 15, 2016

I suggest you change the declaration to use LinkedList instead of the cast here

+
+ private static final Logger LOGGER = LoggerFactory.getLogger(InMemoryClientConnector.class);
+ private long sleepMillis = 0;
+ private final ServerConnector serverConnector;
@blackdrag
blackdrag Sep 15, 2016

why the unusual placement of these at the end of the class?

@hendrikebbers hendrikebbers changes based on review
256f959
@hendrikebbers
Member

Review status: 0 of 15 files reviewed at latest revision, 3 unresolved discussions.


remoting/dolphin-remoting-client/src/main/groovy/org/opendolphin/core/client/comm/CommandBatcher.groovy, line 34 at r1 (raw file):

Previously, blackdrag (Jochen Theodorou) wrote…

why not move this to its own class and base the implementation on one of the Java Queues?

Created a class for the queue. Implementation is still the same. I think we can do this in the next iteration

remoting/dolphin-remoting-combined/src/main/groovy/org/opendolphin/core/client/comm/InMemoryClientConnector.groovy, line 49 at r1 (raw file):

Previously, blackdrag (Jochen Theodorou) wrote…

I suggest you change the declaration to use LinkedList instead of the cast here

Done

remoting/dolphin-remoting-combined/src/main/groovy/org/opendolphin/core/client/comm/InMemoryClientConnector.groovy, line 66 at r1 (raw file):

Previously, blackdrag (Jochen Theodorou) wrote…

why the unusual placement of these at the end of the class?

Done

Comments from Reviewable

@coveralls

Coverage Status

Coverage decreased (-0.06%) to 68.968% when pulling 256f959 on gpars-removed into 1cd72c3 on master.

@hendrikebbers hendrikebbers merged commit 42dd60e into master Oct 17, 2016

2 of 4 checks passed

coverage/coveralls Coverage decreased (-0.06%) to 68.968%
Details
code-review/reviewable 16 files, 3 discussions left (blackdrag)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@aalmiray aalmiray removed the in progress label Oct 17, 2016
@hendrikebbers hendrikebbers deleted the gpars-removed branch Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment