@hachikuji @jsancio Do you know why this was a Set? It seems to me that a Map makes more sense, but I thought I'd ask for a second opinion and I know you have made changes to related code recently.

Agree, a Map makes more sense. Thanks for the change.

I think the suggested way of doing this do that it has the same behaviour in Scala 2.11, 2.12 and 2.13 is ...asScala.view.filterKeys(_.startsWith("...")).toMap.

Views have a lot of problems in 2.11 and 2.12 so we generally stay away from them (they were completely rewritten in Scala 2.13). For code like this where performance doesn't matter, it's fine to go with the simple approach.

Do you remember what are these issues?
I am aware of GC pressure for long lived "view" objects since they hold on to the underlining collection. I don't think this issue applies here since filterKeys creates a "view" which is converted to a strict Map and we never store a reference to the "view".

There are several bugs, particularly in 2.11. You can search the bug tracker if you like. People working on the standard library don't recommend views, they suggest iterator instead. The migration notes for 2.13 also don't mention views as the replacement for the idiom, the example given:

https://docs.scala-lang.org/overviews/core/collections-migration-213.html

A recent article talks about two issues: https://www.scala-lang.org/blog/2017/11/28/view-based-collections.html

I also recall an article showing that views didn't even perform better than the strict equivalent in many scenarios they were designed for.

Personally, I don't think it's worth the additional complexity (particularly around debugging).

Okay. Thanks for the information. The deprecation notice for filterKeys says:

(Since version 2.13.0) Use .view.filterKeys(f). A future version will include a strict version of this method (for now, .view.filterKeys(p).toMap).

Looks like there may be some inconsistency in the documentation. Like you say, in this particular code it doesn't matter much but maybe we should do what you did in a few other places:

scalaMap.filter { case (key, value) => predicate(key) }

OK, I changed filterKeys to filter here and a few other places.

This can be simplified by merging these two lines to getCommittedOffsets(groupId).keys.toSeq.

I think importing scala.collection.{Set, Seq, Map, etc} causes a lot of issues. I think this is going to be even more problematic with 2.13. I think we should ban these imports and only allow the Predef imports, scala.collection.mutable and scala.collection.immutable. What do you think?

Can you elaborate on the issues you are thinking of? The Predef imports are all about immutable collections and they have different performance characteristics and I don't think it makes sense to make that change for Scala 2.13 support.

If we take Map as an example, the issue is that collection.Map, collection.immutable.Map and collection.mutable.Map are all different types. collection.Map is a parent both mutable and immutable but the reality is that collection.Map cannot be safely downcasted to collection.immutable.Map or collection.mutable.Map.

[info] Compiling 1 Scala source to target/scala-2.12/classes ...
[error] TestCollection.scala:16:22: type mismatch;
[error]  found   : Map[String,String] (in scala.collection)
[error]  required: Map[String,String] (in scala.collection.immutable)
[error]   expectImmutableMap(map)
[error]                      ^
[error] TestCollection.scala:18:22: type mismatch;
[error]  found   : Map[String,String] (in scala.collection.mutable)
[error]  required: Map[String,String] (in scala.collection.immutable)
[error]   expectImmutableMap(mutableMap)
[error]                      ^
[error] TestCollection.scala:20:20: type mismatch;
[error]  found   : Map[String,String] (in scala.collection)
[error]  required: Map[String,String] (in scala.collection.mutable)
[error]   expectMutableMap(map)
[error]                    ^
[error] TestCollection.scala:21:20: type mismatch;
[error]  found   : Map[String,String] (in scala.collection.immutable)
[error]  required: Map[String,String] (in scala.collection.mutable)
[error]   expectMutableMap(immutableMap)
[error]                    ^
[error] four errors found

It is very rare that we have code that is generic over mutable vs immutable collection. I think that this conversions are simplify is we require a concrete version like collection.immtable.Map or collection.mutable.Map.

The Predef imports are all about immutable collections

This is true except for Seq but they are fixing this in Scala 2.13. In the other Scala version Seq was a alias for collection.Seq which is the parent class for collection.immutable.Seq and collection.mutable.Seq.

I think it's worth understanding the context. The Kafka code was written with the assumption that the Seq in Predef was not immutable. In Scala 2.13 it has become immutable. The simplest change to maintain existing assumptions is to add import scala.collection.Seq everywhere. In the future, we can consider other changes, I don't think they should be in the Scala 2.13 PR.

In this particular class, I did add an import to scala.collection.Map and I can remove that if that's what you are objecting to.

It is very rare that we have code that is generic over mutable vs immutable collection.

I don't think this is true, a lot of code just does map/filter/flatMap/foreach/etc. Such code doesn't care whether the collection is mutable or immutable.

but the reality is that collection.Map cannot be safely downcasted to collection.immutable.Map or collection.mutable.Map

I don't think you should ever downcast collections outside of very specialized collection utility code (e.g. a method that converts from one collection to another could do this for best performance).

👍

Here also. I think the recommend way to guarantee the same behaviour across Scala version (lazy eval) is to do ...all.get().asScala.view.mapValues(_.asScala).toMap

I responded to the other comment. Let's discuss it there and we'll do here whatever we agree there.

How about proposedReplicaAssignment.view.filterKeys(...).toMap?

See my comment about views being discouraged in Scala 2.11 and Scala 2.12.

👍

How about newConfig.originals.asScala.view.filterKeys(...).filterKeys(...).toMap?

Let's discuss this in the other comment about views. :)

I find that one line imports like some.package.{One, Two, Three} lead to unnecessary merge conflicts. How about encouraging one line imports like:

import some.package.One
import some.package.Two
...

This is the current style. If we want to change it, let's start a discussion in the mailing list.