Hi Till, +1 to define an interface and load it at runtime if we can do. No disrupting the workflows of devs and throw an exception with good description look good to me. This also force us to do a good dependent class abstract.
Best, Jingsong Lee On Wed, Apr 15, 2020 at 10:31 PM Till Rohrmann <trohrm...@apache.org> wrote: > Hi everyone, > > thanks for starting this discussion Niels. > > I like the idea of getting rid of parsing a Properties instance. On the > other hand, I also understand that people are concerned about disrupting > the workflows of our devs. > > Maybe we can find a compromise between both approaches. For example, one > could define an EnvironmentInformationProvider interface which is loaded at > runtime. The implementation of this interface could be generated by `mvn > generate-sources`. Moreover, if we cannot find it, then we fail with a > descriptive exception saying to run `mvn generate-sources`. That way only > devs who rely on the EnvironmentInformation have to run `mvn > generate-sources` when testing/developing and we could still get rid of all > the parsing and logic of a properties file. What do you think? > > Cheers, > Till > > On Mon, Apr 13, 2020 at 10:53 AM Niels Basjes <ni...@basjes.nl> wrote: > > > Hi, > > > > On Mon, Apr 13, 2020 at 8:30 AM Yang Wang <danrtsey...@gmail.com> wrote: > > > > > Although the latter option is more stable, > > > i think it is not acceptable for all the developers to execute `mvn > > > generate-sources` first. > > > > Otherwise, the Flink project is just broken and could not run tests, > Flink > > > jobs in IDE. > > > > > > > > It is important to realize that this discussion is essentially around the > > fact that systems like IntelliJ (what I use) execute various ways of > > generating code that have been written in a maven pom.xml in different > > ways. Simply put: They don't execute all instructions that have been > > defined in the pom.xml automatically when needed. > > > > - With the properties file variant (which uses resource filtering) > > IntelliJ does resource filtering automatically yet no maven plugins > are > > run that affect the properties that are replaced. > > So the git variables are NOT populated when running in IntelliJ and I > > have seen several values in the propersites file are nonsense. > > As a consequence the Java code reading those needs to catch things > like > > "missing properties file" and various kinds of "nonsense values" and > > replace them with a workable default. > > So when running this code you are actually running something different > > from what will be run when doing a `mvn clean install`, yet the > > developer > > is under the illusion it all works because of the code that catches > all > > of > > those problems. > > Depending on the way these variables are used this may lead to "It > fails > > in production but it works fine in IntelliJ" situations. > > In my mind the code that catches all of those exceptional situations > in > > poorly generated properties only exists so that developers can run the > > (otherwise perfectly fine) software in a "broken" build/development > > environment. > > > > > > - With the way I propose to generate the code the effect is indeed > > slightly harder: If you do not run the pom.xml based code generation > it > > will not work. > > I understand that this requires the developers to think a bit more > about > > the code they are working on. > > The upside is that the code either works perfectly or does not > compile. > > There is no "it compiles but is really nonsense". > > I prefer this. > > Also at this point in time this is already true for Flink: There are > > quite a few modules where the developer MUST run mvn generate-sources > > for > > all tests to run successfully. > > Like I indicated there is a javacc SQL parser and there are a lot of > > tests that rely on generating Avro code before they are able to run. > > I have several projects where systems like Avro and Antlr force me in > > this direction, there is simply no other way to do this. > > > > So i think the version properties file is enough for now. +1 for the > first > > > option. > > > > > > > Like I said. I'm fine with either choice by the committers. > > I would choose the `mvn generate-sources` code variant as it is much > > simpler and much more reliable. > > > > Niels > > > > Niels Basjes <ni...@basjes.nl> 于2020年4月9日周四 下午4:47写道： > > > > > > > Hi, > > > > > > > > I'm working on https://issues.apache.org/jira/browse/FLINK-16871 to > > make > > > > more build time variables (like the scala version) into the code > > > available > > > > at runtime. > > > > > > > > During the review process there was discussion around a basic > question: > > > *Is > > > > generating java code during the build ok?* > > > > See > > > > > > > > - > https://github.com/apache/flink/pull/11245#discussion_r400035133 > > > > - https://github.com/apache/flink/pull/11592 > > > > - > https://github.com/apache/flink/pull/11592#issuecomment-610282450 > > > > > > > > As suggested by Chesnay Schepler I'm putting this question to the > > mailing > > > > list. > > > > https://github.com/apache/flink/pull/11592#issuecomment-610963947 > > > > > > > > The main discussion was around the ease of use when running in an IDE > > > like > > > > IntelliJ. > > > > > > > > So essentially we have two solution directions available: > > > > > > > > 1. *Generate a properties file and then use the classloader to > load > > > this > > > > file as a resource and then parse it as a property file.* > > > > This is the currently used solution direction for this part of the > > > code. > > > > A rough version of this (to be improved) : > > > > > > > > > > > > > > https://github.com/apache/flink/commit/47099f663b7644056e9d87b262cd4dba034f513e > > > > This method has several effects: > > > > 1. The developer can run the project immediately from within > the > > > IDE > > > > as fallback values are provided if the 'actual' values are > > missing. > > > > 2. This property file (with stuff that should never be > > overwritten) > > > > can be modified by placing a different one in the classpath. In > > > > fact it IS > > > > modified in the flink-dist as it generates a new file with the > > same > > > > name > > > > into the binary distribution (I consider this to be bad). > > > > 3. Loading resources means loading, parsing and a lot of error > > > > handling. Lots of things "can be null" or be a default value. > So > > > the > > > > values are unreliable and lots of code needs to handle this. In > > > fact > > > > when > > > > running from IntelliJ the properties file is generated poorly > > most > > > > of the > > > > time, only during a normal maven build will it work correctly. > > > > 2. *Generate a Java source file and then simply compile this and > > make > > > it > > > > part of the project.* > > > > Essentially the same model as you would have when using Apache > Avro, > > > > Protobuf, Antlr 4 and javacc (several of those are used in > Flink!). > > > > A rough version of this (to be improved) : > > > > > > > > > > > > > > https://github.com/apache/flink/commit/d215e4df60dc9d647dcee1aa9a2114cbf49d0566 > > > > This method has several effects: > > > > 1. The developer MUST run 'mvn generate-sources' before the actual > > the > > > > project immediately from within the IDE as fallback values are > > > > provided if > > > > the 'actual' values are missing. > > > > 2. The code/test will not run until this step is done. > > > > 3. Because the file is generated by a plugin it is always > > correct. > > > As > > > > a consequence all variables are always available and the > > downstream > > > > users > > > > no longer have to handle the "can be null" or "default value" > > > > situations. > > > > > > > > So is generating code similar to what I created a desired change? > > > > My opinion is that it is the better solution, the data available is > > more > > > > reliable and as a consequence the rest of the code is simpler. > > > > It would probably mean that during development of flink you should be > > > aware > > > > of this and do an 'extra step' to get it running. > > > > > > > > What do you guys think? > > > > > > > > -- > > > > Best regards / Met vriendelijke groeten, > > > > > > > > Niels Basjes > > > > > > > > > > > > > -- > > Best regards / Met vriendelijke groeten, > > > > Niels Basjes > > > -- Best, Jingsong Lee
